Re: [PATCH v3 3/4] drivers/net/virtio_net: Added RSS hash report.

Hi all,

On Tue, Feb 8, 2022 at 10:55 PM Willem de Bruijn
[off-list ref] wrote:

On Tue, Feb 8, 2022 at 1:19 PM Andrew Melnychenko [off-list ref] wrote:

quoted

Added features for RSS hash report.
If hash is provided - it sets to skb.
Added checks if rss and/or hash are enabled together.

Signed-off-by: Andrew Melnychenko <redacted>
---
 drivers/net/virtio_net.c | 51 ++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 495aed524e33..543da2fbdd2d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -227,6 +227,7 @@ struct virtnet_info {

        /* Host supports rss and/or hash report */
        bool has_rss;
+       bool has_rss_hash_report;
        u8 rss_key_size;
        u16 rss_indir_table_size;
        u32 rss_hash_types_supported;

@@ -421,7 +422,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,

        hdr_len = vi->hdr_len;
        if (vi->mergeable_rx_bufs)
-               hdr_padded_len = sizeof(*hdr);
+               hdr_padded_len = hdr_len;

Belongs in patch 1?

Yeah, I'll move it.

quoted

        else
                hdr_padded_len = sizeof(struct padded_vnet_hdr);

@@ -1156,6 +1157,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
        struct net_device *dev = vi->dev;
        struct sk_buff *skb;
        struct virtio_net_hdr_mrg_rxbuf *hdr;
+       struct virtio_net_hdr_v1_hash *hdr_hash;
+       enum pkt_hash_types rss_hash_type;

        if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
                pr_debug("%s: short packet %i\n", dev->name, len);

@@ -1182,6 +1185,29 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
                return;

        hdr = skb_vnet_hdr(skb);
+       if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) {

Can the first be true if the second is not?

Yes, RSS may be enabled, but the hash report feature is disabled.
For now, it's possible to enable/disable VirtioNet RSS by manipulating RXHASH.

quoted

+               hdr_hash = (struct virtio_net_hdr_v1_hash *)(hdr);
+
+               switch (hdr_hash->hash_report) {
+               case VIRTIO_NET_HASH_REPORT_TCPv4:
+               case VIRTIO_NET_HASH_REPORT_UDPv4:
+               case VIRTIO_NET_HASH_REPORT_TCPv6:
+               case VIRTIO_NET_HASH_REPORT_UDPv6:
+               case VIRTIO_NET_HASH_REPORT_TCPv6_EX:
+               case VIRTIO_NET_HASH_REPORT_UDPv6_EX:
+                       rss_hash_type = PKT_HASH_TYPE_L4;
+                       break;
+               case VIRTIO_NET_HASH_REPORT_IPv4:
+               case VIRTIO_NET_HASH_REPORT_IPv6:
+               case VIRTIO_NET_HASH_REPORT_IPv6_EX:
+                       rss_hash_type = PKT_HASH_TYPE_L3;
+                       break;
+               case VIRTIO_NET_HASH_REPORT_NONE:
+               default:
+                       rss_hash_type = PKT_HASH_TYPE_NONE;
+               }
+               skb_set_hash(skb, hdr_hash->hash_value, rss_hash_type);
+       }

so many lines, perhaps deserves a helper function

Ok, I'll create the helper.

quoted

        if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
                skb->ip_summed = CHECKSUM_UNNECESSARY;

@@ -2232,7 +2258,8 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
        sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size);

        if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
-                                 VIRTIO_NET_CTRL_MQ_RSS_CONFIG, sgs)) {
+                                 vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
+                                 : VIRTIO_NET_CTRL_MQ_HASH_CONFIG, sgs)) {
                dev_warn(&dev->dev, "VIRTIONET issue with committing RSS sgs\n");
                return false;
        }

@@ -3230,6 +3257,8 @@ static bool virtnet_validate_features(struct virtio_device *vdev)
             VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR,
                             "VIRTIO_NET_F_CTRL_VQ") ||
             VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_RSS,
+                            "VIRTIO_NET_F_CTRL_VQ") ||
+            VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_HASH_REPORT,
                             "VIRTIO_NET_F_CTRL_VQ"))) {
                return false;
        }

@@ -3365,8 +3394,13 @@ static int virtnet_probe(struct virtio_device *vdev)
        if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
                vi->mergeable_rx_bufs = true;

-       if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) {
+       if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
+               vi->has_rss_hash_report = true;
+
+       if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS))
                vi->has_rss = true;
+
+       if (vi->has_rss || vi->has_rss_hash_report) {
                vi->rss_indir_table_size =
                        virtio_cread16(vdev, offsetof(struct virtio_net_config,

should indir table size be zero if only hash report is enabled?

Not really - but of course, for hash only, the table is not necessary.
(Qemu always provides the table with size 1, I'll add checks for zero sizes
in case of hardware implementation.)

quoted

                                rss_max_indirection_table_length));

@@ -3382,8 +3416,11 @@ static int virtnet_probe(struct virtio_device *vdev)

                dev->hw_features |= NETIF_F_RXHASH;
        }
-       if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) ||
-           virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
+
+       if (vi->has_rss_hash_report)
+               vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
+       else if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) ||
+                virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
                vi->hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
        else
                vi->hdr_len = sizeof(struct virtio_net_hdr);

@@ -3450,7 +3487,7 @@ static int virtnet_probe(struct virtio_device *vdev)
                }
        }

-       if (vi->has_rss)
+       if (vi->has_rss || vi->has_rss_hash_report)
                virtnet_init_default_rss(vi);

        err = register_netdev(dev);

@@ -3585,7 +3622,7 @@ static struct virtio_device_id id_table[] = {
        VIRTIO_NET_F_CTRL_MAC_ADDR, \
        VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
        VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
-       VIRTIO_NET_F_RSS
+       VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT

 static unsigned int features[] = {
        VIRTNET_FEATURES,
--

2.34.1