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

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;
> +	}
> +

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

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