[PATCH net 0/4] virtio_net: Make RSS interact properly with queue number

[PATCH net 0/4] virtio_net: Make RSS interact properly with queue number

[Philo Lu]

With this patch set, RSS updates with queue_pairs changing:
- When virtnet_probe, init default rss and commit
- When queue_pairs changes _without_ user rss configuration, update rss
  with the new queue number
- When queue_pairs changes _with_ user rss configuration, keep rss as user
  configured

Patch 1 and 2 fix possible out of bound errors for indir_table and key.
Patch 3 and 4 add RSS update in probe() and set_queues().

Please review, thanks.

Philo Lu (4):
  virtio_net: Support dynamic rss indirection table size
  virtio_net: Add hash_key_length check
  virtio_net: Sync rss config to device when virtnet_probe
  virtio_net: Update rss when set queue

 drivers/net/virtio_net.c | 119 ++++++++++++++++++++++++++++++++-------
 1 file changed, 100 insertions(+), 19 deletions(-)

--
2.32.0.3.g01195cf9f

[PATCH net 1/4] virtio_net: Support dynamic rss indirection table size

[Philo Lu]

When reading/writing virtio_net_ctrl_rss, we get the indirection table
size from vi->rss_indir_table_size, which is initialized in
virtnet_probe(). However, the actual size of indirection_table was set
as VIRTIO_NET_RSS_MAX_TABLE_LEN=128. This collision may cause issues if
the vi->rss_indir_table_size exceeds 128.

This patch instead uses dynamic indirection table, allocated with
vi->rss after vi->rss_indir_table_size initialized. And free it in
virtnet_remove().

In virtnet_commit_rss_command(), sgs for rss is initialized differently
with hash_report. So indirection_table is not used if !vi->has_rss, and
then we don't need to alloc indirection_table for hash_report only uses.

Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 869586c17ffd..75c1ff4efd13 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -368,15 +368,16 @@ struct receive_queue {
  * because table sizes may be differ according to the device configuration.
  */
 #define VIRTIO_NET_RSS_MAX_KEY_SIZE     40
-#define VIRTIO_NET_RSS_MAX_TABLE_LEN    128
 struct virtio_net_ctrl_rss {
 	u32 hash_types;
 	u16 indirection_table_mask;
 	u16 unclassified_queue;
-	u16 indirection_table[VIRTIO_NET_RSS_MAX_TABLE_LEN];
+	u16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
 	u16 max_tx_vq;
 	u8 hash_key_length;
 	u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
+
+	u16 *indirection_table;
 };
 
 /* Control VQ buffers: protected by the rtnl lock */
@@ -512,6 +513,25 @@ static struct sk_buff *virtnet_skb_append_frag(struct sk_buff *head_skb,
 					       struct page *page, void *buf,
 					       int len, int truesize);
 
+static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
+{
+	if (!indir_table_size) {
+		rss->indirection_table = NULL;
+		return 0;
+	}
+
+	rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
+	if (!rss->indirection_table)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
+{
+	kfree(rss->indirection_table);
+}
+
 static bool is_xdp_frame(void *ptr)
 {
 	return (unsigned long)ptr & VIRTIO_XDP_FLAG;
@@ -3828,11 +3848,15 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
 	/* prepare sgs */
 	sg_init_table(sgs, 4);
 
-	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table);
+	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
 	sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
 
-	sg_buf_size = sizeof(uint16_t) * (vi->rss.indirection_table_mask + 1);
-	sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
+	if (vi->has_rss) {
+		sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
+		sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
+	} else {
+		sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
+	}
 
 	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
 			- offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
@@ -6420,6 +6444,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 			virtio_cread16(vdev, offsetof(struct virtio_net_config,
 				rss_max_indirection_table_length));
 	}
+	err = rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size);
+	if (err)
+		goto free;
 
 	if (vi->has_rss || vi->has_rss_hash_report) {
 		vi->rss_key_size =
@@ -6674,6 +6701,8 @@ static void virtnet_remove(struct virtio_device *vdev)
 
 	remove_vq_common(vi);
 
+	rss_indirection_table_free(&vi->rss);
+
 	free_netdev(vi->dev);
 }
 
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net 1/4] virtio_net: Support dynamic rss indirection table size

[Joe Damato]

On Mon, Nov 04, 2024 at 04:57:03PM +0800, Philo Lu wrote:
> When reading/writing virtio_net_ctrl_rss, we get the indirection table
> size from vi->rss_indir_table_size, which is initialized in
> virtnet_probe(). However, the actual size of indirection_table was set
> as VIRTIO_NET_RSS_MAX_TABLE_LEN=128. This collision may cause issues if
> the vi->rss_indir_table_size exceeds 128.
> 
> This patch instead uses dynamic indirection table, allocated with
> vi->rss after vi->rss_indir_table_size initialized. And free it in
> virtnet_remove().
> 
> In virtnet_commit_rss_command(), sgs for rss is initialized differently
> with hash_report. So indirection_table is not used if !vi->has_rss, and
> then we don't need to alloc indirection_table for hash_report only uses.
> 
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 869586c17ffd..75c1ff4efd13 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -368,15 +368,16 @@ struct receive_queue {
>   * because table sizes may be differ according to the device configuration.
>   */
>  #define VIRTIO_NET_RSS_MAX_KEY_SIZE     40
> -#define VIRTIO_NET_RSS_MAX_TABLE_LEN    128
>  struct virtio_net_ctrl_rss {
>  	u32 hash_types;
>  	u16 indirection_table_mask;
>  	u16 unclassified_queue;
> -	u16 indirection_table[VIRTIO_NET_RSS_MAX_TABLE_LEN];
> +	u16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
>  	u16 max_tx_vq;
>  	u8 hash_key_length;
>  	u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> +
> +	u16 *indirection_table;
>  };
>  
>  /* Control VQ buffers: protected by the rtnl lock */
> @@ -512,6 +513,25 @@ static struct sk_buff *virtnet_skb_append_frag(struct sk_buff *head_skb,
>  					       struct page *page, void *buf,
>  					       int len, int truesize);
>  
> +static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
> +{
> +	if (!indir_table_size) {
> +		rss->indirection_table = NULL;
> +		return 0;
> +	}
> +
> +	rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
> +	if (!rss->indirection_table)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
> +{
> +	kfree(rss->indirection_table);
> +}
> +
>  static bool is_xdp_frame(void *ptr)
>  {
>  	return (unsigned long)ptr & VIRTIO_XDP_FLAG;
> @@ -3828,11 +3848,15 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>  	/* prepare sgs */
>  	sg_init_table(sgs, 4);
>  
> -	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table);
> +	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
>  	sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
>  
> -	sg_buf_size = sizeof(uint16_t) * (vi->rss.indirection_table_mask + 1);
> -	sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
> +	if (vi->has_rss) {
> +		sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
> +		sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
> +	} else {
> +		sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
> +	}
>  
>  	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
>  			- offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> @@ -6420,6 +6444,9 @@ static int virtnet_probe(struct virtio_device *vdev)
>  			virtio_cread16(vdev, offsetof(struct virtio_net_config,
>  				rss_max_indirection_table_length));
>  	}
> +	err = rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size);
> +	if (err)
> +		goto free;
>  
>  	if (vi->has_rss || vi->has_rss_hash_report) {
>  		vi->rss_key_size =
> @@ -6674,6 +6701,8 @@ static void virtnet_remove(struct virtio_device *vdev)
>  
>  	remove_vq_common(vi);
>  
> +	rss_indirection_table_free(&vi->rss);
> +
>  	free_netdev(vi->dev);
>  }
>

I'm not an expert on virtio, so I don't feel comfortable giving a
Reviewed-by, but this does seem to fix a potential out of bounds
access in virtnet_init_default_rss if rss_indir_table_size were
larger than VIRTIO_NET_RSS_MAX_TABLE_LEN (128).

Acked-by: Joe Damato <jdamato@fastly.com>

[PATCH net 2/4] virtio_net: Add hash_key_length check

[Philo Lu]

Add hash_key_length check in virtnet_probe() to avoid possible out of
bound errors when setting/reading the hash key.

Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 75c1ff4efd13..acc3e5dc112e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -6451,6 +6451,12 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (vi->has_rss || vi->has_rss_hash_report) {
 		vi->rss_key_size =
 			virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
+		if (vi->rss_key_size > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
+			dev_err(&vdev->dev, "rss_max_key_size=%u exceeds the limit %u.\n",
+				vi->rss_key_size, VIRTIO_NET_RSS_MAX_KEY_SIZE);
+			err = -EINVAL;
+			goto free;
+		}
 
 		vi->rss_hash_types_supported =
 		    virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types));
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net 2/4] virtio_net: Add hash_key_length check

[Joe Damato]

On Mon, Nov 04, 2024 at 04:57:04PM +0800, Philo Lu wrote:
> Add hash_key_length check in virtnet_probe() to avoid possible out of
> bound errors when setting/reading the hash key.
> 
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 75c1ff4efd13..acc3e5dc112e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -6451,6 +6451,12 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	if (vi->has_rss || vi->has_rss_hash_report) {
>  		vi->rss_key_size =
>  			virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
> +		if (vi->rss_key_size > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
> +			dev_err(&vdev->dev, "rss_max_key_size=%u exceeds the limit %u.\n",
> +				vi->rss_key_size, VIRTIO_NET_RSS_MAX_KEY_SIZE);
> +			err = -EINVAL;
> +			goto free;
> +		}

I agree that an out of bounds error could occur and a check here
is needed.

I have no idea if returning -EINVAL from probe is the correct
solution (vs say using min()) as I am just a casual observer of
virtio_net and not a maintainer.

Acked-by: Joe Damato <jdamato@fastly.com>

[PATCH net 3/4] virtio_net: Sync rss config to device when virtnet_probe

[Philo Lu]

During virtnet_probe, default rss configuration is initialized, but was
not committed to the device. This patch fix this by sending rss command
after device ready in virtnet_probe. Otherwise, the actual rss
configuration used by device can be different with that read by user
from driver, which may confuse the user.

If the command committing fails, driver rss will be disabled.

Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index acc3e5dc112e..59d9fdf562e0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -6584,6 +6584,15 @@ static int virtnet_probe(struct virtio_device *vdev)
 
 	virtio_device_ready(vdev);
 
+	if (vi->has_rss || vi->has_rss_hash_report) {
+		if (!virtnet_commit_rss_command(vi)) {
+			dev_warn(&vdev->dev, "RSS disabled because committing failed.\n");
+			dev->hw_features &= ~NETIF_F_RXHASH;
+			vi->has_rss_hash_report = false;
+			vi->has_rss = false;
+		}
+	}
+
 	virtnet_set_queues(vi, vi->curr_queue_pairs);
 
 	/* a random MAC address has been assigned, notify the device.
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net 3/4] virtio_net: Sync rss config to device when virtnet_probe

[Joe Damato]

On Mon, Nov 04, 2024 at 04:57:05PM +0800, Philo Lu wrote:
> During virtnet_probe, default rss configuration is initialized, but was
> not committed to the device. This patch fix this by sending rss command
> after device ready in virtnet_probe. Otherwise, the actual rss
> configuration used by device can be different with that read by user
> from driver, which may confuse the user.
> 
> If the command committing fails, driver rss will be disabled.
> 
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index acc3e5dc112e..59d9fdf562e0 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -6584,6 +6584,15 @@ static int virtnet_probe(struct virtio_device *vdev)
>  
>  	virtio_device_ready(vdev);
>  
> +	if (vi->has_rss || vi->has_rss_hash_report) {
> +		if (!virtnet_commit_rss_command(vi)) {
> +			dev_warn(&vdev->dev, "RSS disabled because committing failed.\n");
> +			dev->hw_features &= ~NETIF_F_RXHASH;
> +			vi->has_rss_hash_report = false;
> +			vi->has_rss = false;
> +		}
> +	}
> +
>  	virtnet_set_queues(vi, vi->curr_queue_pairs);
>  
>  	/* a random MAC address has been assigned, notify the device.

Acked-by: Joe Damato <jdamato@fastly.com>

[PATCH net 4/4] virtio_net: Update rss when set queue

[Philo Lu]

RSS configuration should be updated with queue number. In particular, it
should be updated when (1) rss enabled and (2) default rss configuration
is used without user modification.

During rss command processing, device updates queue_pairs using
rss.max_tx_vq. That is, the device updates queue_pairs together with
rss, so we can skip the sperate queue_pairs update
(VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.

Also remove the `vi->has_rss ?` check when setting vi->rss.max_tx_vq,
because this is not used in the other hash_report case.

Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 65 +++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 14 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 59d9fdf562e0..189afad3ffaa 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3394,15 +3394,59 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
 		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
 }
 
+static bool virtnet_commit_rss_command(struct virtnet_info *vi);
+
+static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pairs)
+{
+	u32 indir_val = 0;
+	int i = 0;
+
+	for (; i < vi->rss_indir_table_size; ++i) {
+		indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
+		vi->rss.indirection_table[i] = indir_val;
+	}
+	vi->rss.max_tx_vq = queue_pairs;
+}
+
 static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 {
 	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
-	struct scatterlist sg;
+	struct virtio_net_ctrl_rss old_rss;
 	struct net_device *dev = vi->dev;
+	struct scatterlist sg;
 
 	if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
 		return 0;
 
+	/* Firstly check if we need update rss. Do updating if both (1) rss enabled and
+	 * (2) no user configuration.
+	 *
+	 * During rss command processing, device updates queue_pairs using rss.max_tx_vq. That is,
+	 * the device updates queue_pairs together with rss, so we can skip the sperate queue_pairs
+	 * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
+	 */
+	if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
+		memcpy(&old_rss, &vi->rss, sizeof(old_rss));
+		if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
+			vi->rss.indirection_table = old_rss.indirection_table;
+			return -ENOMEM;
+		}
+
+		virtnet_rss_update_by_qpairs(vi, queue_pairs);
+
+		if (!virtnet_commit_rss_command(vi)) {
+			/* restore ctrl_rss if commit_rss_command failed */
+			rss_indirection_table_free(&vi->rss);
+			memcpy(&vi->rss, &old_rss, sizeof(old_rss));
+
+			dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
+				 queue_pairs);
+			return -EINVAL;
+		}
+		rss_indirection_table_free(&old_rss);
+		goto succ;
+	}
+
 	mq = kzalloc(sizeof(*mq), GFP_KERNEL);
 	if (!mq)
 		return -ENOMEM;
@@ -3415,12 +3459,12 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 		dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
 			 queue_pairs);
 		return -EINVAL;
-	} else {
-		vi->curr_queue_pairs = queue_pairs;
-		/* virtnet_open() will refill when device is going to up. */
-		if (dev->flags & IFF_UP)
-			schedule_delayed_work(&vi->refill, 0);
 	}
+succ:
+	vi->curr_queue_pairs = queue_pairs;
+	/* virtnet_open() will refill when device is going to up. */
+	if (dev->flags & IFF_UP)
+		schedule_delayed_work(&vi->refill, 0);
 
 	return 0;
 }
@@ -3880,21 +3924,14 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
 
 static void virtnet_init_default_rss(struct virtnet_info *vi)
 {
-	u32 indir_val = 0;
-	int i = 0;
-
 	vi->rss.hash_types = vi->rss_hash_types_supported;
 	vi->rss_hash_types_saved = vi->rss_hash_types_supported;
 	vi->rss.indirection_table_mask = vi->rss_indir_table_size
 						? vi->rss_indir_table_size - 1 : 0;
 	vi->rss.unclassified_queue = 0;
 
-	for (; i < vi->rss_indir_table_size; ++i) {
-		indir_val = ethtool_rxfh_indir_default(i, vi->curr_queue_pairs);
-		vi->rss.indirection_table[i] = indir_val;
-	}
+	virtnet_rss_update_by_qpairs(vi, vi->curr_queue_pairs);
 
-	vi->rss.max_tx_vq = vi->has_rss ? vi->curr_queue_pairs : 0;
 	vi->rss.hash_key_length = vi->rss_key_size;
 
 	netdev_rss_key_fill(vi->rss.key, vi->rss_key_size);
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net 4/4] virtio_net: Update rss when set queue

[Joe Damato]

On Mon, Nov 04, 2024 at 04:57:06PM +0800, Philo Lu wrote:
> RSS configuration should be updated with queue number. In particular, it
> should be updated when (1) rss enabled and (2) default rss configuration
> is used without user modification.
> 
> During rss command processing, device updates queue_pairs using
> rss.max_tx_vq. That is, the device updates queue_pairs together with
> rss, so we can skip the sperate queue_pairs update
> (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
> 
> Also remove the `vi->has_rss ?` check when setting vi->rss.max_tx_vq,
> because this is not used in the other hash_report case.
> 
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 65 +++++++++++++++++++++++++++++++---------
>  1 file changed, 51 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 59d9fdf562e0..189afad3ffaa 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3394,15 +3394,59 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>  		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>  }
>  
> +static bool virtnet_commit_rss_command(struct virtnet_info *vi);
> +
> +static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pairs)
> +{
> +	u32 indir_val = 0;
> +	int i = 0;
> +
> +	for (; i < vi->rss_indir_table_size; ++i) {
> +		indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
> +		vi->rss.indirection_table[i] = indir_val;
> +	}
> +	vi->rss.max_tx_vq = queue_pairs;
> +}
> +
>  static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>  {
>  	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> -	struct scatterlist sg;
> +	struct virtio_net_ctrl_rss old_rss;
>  	struct net_device *dev = vi->dev;
> +	struct scatterlist sg;
>  
>  	if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>  		return 0;
>  
> +	/* Firstly check if we need update rss. Do updating if both (1) rss enabled and
> +	 * (2) no user configuration.
> +	 *
> +	 * During rss command processing, device updates queue_pairs using rss.max_tx_vq. That is,
> +	 * the device updates queue_pairs together with rss, so we can skip the sperate queue_pairs
> +	 * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
> +	 */
> +	if (vi->has_rss && !netif_is_rxfh_configured(dev)) {

Does there need to be an error case when:

vi->has_rss && netif_is_rxfh_configured(dev)

to return EINVAL? I noted that other drivers don't let users adjust
the queue count and return error in this case.


> +		memcpy(&old_rss, &vi->rss, sizeof(old_rss));
> +		if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
> +			vi->rss.indirection_table = old_rss.indirection_table;
> +			return -ENOMEM;
> +		}
> +
> +		virtnet_rss_update_by_qpairs(vi, queue_pairs);
> +
> +		if (!virtnet_commit_rss_command(vi)) {
> +			/* restore ctrl_rss if commit_rss_command failed */
> +			rss_indirection_table_free(&vi->rss);
> +			memcpy(&vi->rss, &old_rss, sizeof(old_rss));
> +
> +			dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
> +				 queue_pairs);
> +			return -EINVAL;
> +		}
> +		rss_indirection_table_free(&old_rss);
> +		goto succ;
> +	}
> +

Re: [PATCH net 4/4] virtio_net: Update rss when set queue

[Philo Lu]

On 2024/11/6 04:31, Joe Damato wrote:
> On Mon, Nov 04, 2024 at 04:57:06PM +0800, Philo Lu wrote:
>> RSS configuration should be updated with queue number. In particular, it
>> should be updated when (1) rss enabled and (2) default rss configuration
>> is used without user modification.
>>
>> During rss command processing, device updates queue_pairs using
>> rss.max_tx_vq. That is, the device updates queue_pairs together with
>> rss, so we can skip the sperate queue_pairs update
>> (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>>
>> Also remove the `vi->has_rss ?` check when setting vi->rss.max_tx_vq,
>> because this is not used in the other hash_report case.
>>
>> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
>> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   drivers/net/virtio_net.c | 65 +++++++++++++++++++++++++++++++---------
>>   1 file changed, 51 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 59d9fdf562e0..189afad3ffaa 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -3394,15 +3394,59 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>>   		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>>   }
>>   
>> +static bool virtnet_commit_rss_command(struct virtnet_info *vi);
>> +
>> +static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pairs)
>> +{
>> +	u32 indir_val = 0;
>> +	int i = 0;
>> +
>> +	for (; i < vi->rss_indir_table_size; ++i) {
>> +		indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
>> +		vi->rss.indirection_table[i] = indir_val;
>> +	}
>> +	vi->rss.max_tx_vq = queue_pairs;
>> +}
>> +
>>   static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>   {
>>   	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>> -	struct scatterlist sg;
>> +	struct virtio_net_ctrl_rss old_rss;
>>   	struct net_device *dev = vi->dev;
>> +	struct scatterlist sg;
>>   
>>   	if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>>   		return 0;
>>   
>> +	/* Firstly check if we need update rss. Do updating if both (1) rss enabled and
>> +	 * (2) no user configuration.
>> +	 *
>> +	 * During rss command processing, device updates queue_pairs using rss.max_tx_vq. That is,
>> +	 * the device updates queue_pairs together with rss, so we can skip the sperate queue_pairs
>> +	 * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>> +	 */
>> +	if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
> 
> Does there need to be an error case when:
> 
> vi->has_rss && netif_is_rxfh_configured(dev)
> 
> to return EINVAL? I noted that other drivers don't let users adjust
> the queue count and return error in this case.
> 

In fact, there are 2 possible cases if users have adjusted rss, 
depending on the total queue pairs used in the indirection table (x), 
and the requested new queue count (y).

Case A: If y < x, it's illegal and will be rejected by
         ethtool_check_max_channel().
Case B: If x <= y, we only adjust the queue number without touching the
         rss configuration set by users.

So I don't think it necessary to add the check (if the above processing 
is agreed).

Thanks for your review, Joe.

-- 
Philo

Re: [PATCH net 4/4] virtio_net: Update rss when set queue

[Paolo Abeni]

On 11/5/24 21:31, Joe Damato wrote:
> On Mon, Nov 04, 2024 at 04:57:06PM +0800, Philo Lu wrote:
>> RSS configuration should be updated with queue number. In particular, it
>> should be updated when (1) rss enabled and (2) default rss configuration
>> is used without user modification.
>>
>> During rss command processing, device updates queue_pairs using
>> rss.max_tx_vq. That is, the device updates queue_pairs together with
>> rss, so we can skip the sperate queue_pairs update
>> (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>>
>> Also remove the `vi->has_rss ?` check when setting vi->rss.max_tx_vq,
>> because this is not used in the other hash_report case.
>>
>> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
>> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>  drivers/net/virtio_net.c | 65 +++++++++++++++++++++++++++++++---------
>>  1 file changed, 51 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 59d9fdf562e0..189afad3ffaa 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -3394,15 +3394,59 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>>  		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>>  }
>>  
>> +static bool virtnet_commit_rss_command(struct virtnet_info *vi);
>> +
>> +static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pairs)
>> +{
>> +	u32 indir_val = 0;
>> +	int i = 0;
>> +
>> +	for (; i < vi->rss_indir_table_size; ++i) {
>> +		indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
>> +		vi->rss.indirection_table[i] = indir_val;
>> +	}
>> +	vi->rss.max_tx_vq = queue_pairs;
>> +}
>> +
>>  static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>  {
>>  	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>> -	struct scatterlist sg;
>> +	struct virtio_net_ctrl_rss old_rss;
>>  	struct net_device *dev = vi->dev;
>> +	struct scatterlist sg;
>>  
>>  	if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>>  		return 0;
>>  
>> +	/* Firstly check if we need update rss. Do updating if both (1) rss enabled and
>> +	 * (2) no user configuration.
>> +	 *
>> +	 * During rss command processing, device updates queue_pairs using rss.max_tx_vq. That is,
>> +	 * the device updates queue_pairs together with rss, so we can skip the sperate queue_pairs
>> +	 * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>> +	 */
>> +	if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
> 
> Does there need to be an error case when:
> 
> vi->has_rss && netif_is_rxfh_configured(dev)
> 
> to return EINVAL? I noted that other drivers don't let users adjust
> the queue count and return error in this case.

AFAICS the above is orthogonal to this patch - i.e. lack of check is
pre-existing and not introduced here. I'm not 110% sure the lack of
check is illegit, but I think it should eventually handled with a
separate patch/series.

Thanks,

Paolo

Re: [PATCH net 0/4] virtio_net: Make RSS interact properly with queue number

[Xuan Zhuo]

Hi Jason, could you review this firstly?

Thanks.

On Mon,  4 Nov 2024 16:57:02 +0800, Philo Lu <lulie@linux.alibaba.com> wrote:
> With this patch set, RSS updates with queue_pairs changing:
> - When virtnet_probe, init default rss and commit
> - When queue_pairs changes _without_ user rss configuration, update rss
>   with the new queue number
> - When queue_pairs changes _with_ user rss configuration, keep rss as user
>   configured
>
> Patch 1 and 2 fix possible out of bound errors for indir_table and key.
> Patch 3 and 4 add RSS update in probe() and set_queues().
>
> Please review, thanks.
>
> Philo Lu (4):
>   virtio_net: Support dynamic rss indirection table size
>   virtio_net: Add hash_key_length check
>   virtio_net: Sync rss config to device when virtnet_probe
>   virtio_net: Update rss when set queue
>
>  drivers/net/virtio_net.c | 119 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 100 insertions(+), 19 deletions(-)
>
> --
> 2.32.0.3.g01195cf9f
>

Re: [PATCH net 0/4] virtio_net: Make RSS interact properly with queue number

[Jason Wang]

On Wed, Nov 6, 2024 at 5:00 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> Hi Jason, could you review this firstly?
>
> Thanks.
>

It looks like the series has been merged.

Anyhow it looks good to me.

Thanks

Re: [PATCH net 0/4] virtio_net: Make RSS interact properly with queue number

[Michael S. Tsirkin]

On Mon, Nov 04, 2024 at 04:57:02PM +0800, Philo Lu wrote:
> With this patch set, RSS updates with queue_pairs changing:
> - When virtnet_probe, init default rss and commit
> - When queue_pairs changes _without_ user rss configuration, update rss
>   with the new queue number
> - When queue_pairs changes _with_ user rss configuration, keep rss as user
>   configured
> 
> Patch 1 and 2 fix possible out of bound errors for indir_table and key.
> Patch 3 and 4 add RSS update in probe() and set_queues().
> 
> Please review, thanks.

Looks reasonable.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> Philo Lu (4):
>   virtio_net: Support dynamic rss indirection table size
>   virtio_net: Add hash_key_length check
>   virtio_net: Sync rss config to device when virtnet_probe
>   virtio_net: Update rss when set queue
> 
>  drivers/net/virtio_net.c | 119 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 100 insertions(+), 19 deletions(-)
> 
> --
> 2.32.0.3.g01195cf9f

Re: [PATCH net 0/4] virtio_net: Make RSS interact properly with queue number

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon,  4 Nov 2024 16:57:02 +0800 you wrote:
> With this patch set, RSS updates with queue_pairs changing:
> - When virtnet_probe, init default rss and commit
> - When queue_pairs changes _without_ user rss configuration, update rss
>   with the new queue number
> - When queue_pairs changes _with_ user rss configuration, keep rss as user
>   configured
> 
> [...]

Here is the summary with links:
  - [net,1/4] virtio_net: Support dynamic rss indirection table size
    https://git.kernel.org/netdev/net/c/86a48a00efdf
  - [net,2/4] virtio_net: Add hash_key_length check
    https://git.kernel.org/netdev/net/c/3f7d9c1964fc
  - [net,3/4] virtio_net: Sync rss config to device when virtnet_probe
    https://git.kernel.org/netdev/net/c/dc749b7b0608
  - [net,4/4] virtio_net: Update rss when set queue
    https://git.kernel.org/netdev/net/c/50bfcaedd78e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html