[PATCH net-next] ibmvnic: Assign XPS map to correct queue index

[PATCH net-next] ibmvnic: Assign XPS map to correct queue index

[Nick Child]

When setting the XPS map value for TX queues, use the index of the
transmit queue.
Previously, the function was passing the index of the loop that iterates
over all queues (RX and TX). This was causing invalid XPS map values.

Fixes: 6831582937bd ("ibmvnic: Toggle between queue types in affinity mapping")
Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---

I am a little surprised that __netif_set_xps_queue() did not complain that some
index values were greater than the number of tx queues. Though maybe the function
assumes that the developers are wise enough :)

Should __netif_set_xps_queue() have a check that index < dev->num_tx_queues?

 drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 146ca1d8031b..c63d3ec9d328 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -296,10 +296,10 @@ static void ibmvnic_set_affinity(struct ibmvnic_adapter *adapter)
 
 		rc = __netif_set_xps_queue(adapter->netdev,
 					   cpumask_bits(queue->affinity_mask),
-					   i, XPS_CPUS);
+					   i_txqs - 1, XPS_CPUS);
 		if (rc)
 			netdev_warn(adapter->netdev, "%s: Set XPS on queue %d failed, rc = %d.\n",
-				    __func__, i, rc);
+				    __func__, i_txqs - 1, rc);
 	}
 
 out:
-- 
2.31.1

Re: [PATCH net-next] ibmvnic: Assign XPS map to correct queue index

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Thu, Feb 23, 2023 at 9:10 PM Nick Child <nnac123@linux.ibm.com> wrote:
>
> When setting the XPS map value for TX queues, use the index of the
> transmit queue.
> Previously, the function was passing the index of the loop that iterates
> over all queues (RX and TX). This was causing invalid XPS map values.
>
> Fixes: 6831582937bd ("ibmvnic: Toggle between queue types in affinity mapping")
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>

Fix looks good to me.
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

> ---
>
> I am a little surprised that __netif_set_xps_queue() did not complain that some
> index values were greater than the number of tx queues. Though maybe the function
> assumes that the developers are wise enough :)

Are you sure an index greater than total tx queues was sent?
A quick look at ibmvnic_set_affinity() suggests that the condition if
(... || (i_txqs == num_txqs)) prevents overrunning available tx
queues.

>
> Should __netif_set_xps_queue() have a check that index < dev->num_tx_queues?
>
>  drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 146ca1d8031b..c63d3ec9d328 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -296,10 +296,10 @@ static void ibmvnic_set_affinity(struct ibmvnic_adapter *adapter)
>
>                 rc = __netif_set_xps_queue(adapter->netdev,
>                                            cpumask_bits(queue->affinity_mask),
> -                                          i, XPS_CPUS);
> +                                          i_txqs - 1, XPS_CPUS);
>                 if (rc)
>                         netdev_warn(adapter->netdev, "%s: Set XPS on queue %d failed, rc = %d.\n",
> -                                   __func__, i, rc);
> +                                   __func__, i_txqs - 1, rc);
>         }
>
>  out:
> --
> 2.31.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next] ibmvnic: Assign XPS map to correct queue index

[Nick Child]

On 2/24/23 02:49, Pavan Chebbi wrote:
> On Thu, Feb 23, 2023 at 9:10 PM Nick Child <nnac123@linux.ibm.com> wrote:
>> ---
>>
>> I am a little surprised that __netif_set_xps_queue() did not complain that some
>> index values were greater than the number of tx queues. Though maybe the function
>> assumes that the developers are wise enough :)
> 
> Are you sure an index greater than total tx queues was sent?
> A quick look at ibmvnic_set_affinity() suggests that the condition if
> (... || (i_txqs == num_txqs)) prevents overrunning available tx
> queues.
> 
I believe so. The issue is we were sending "i" to 
__netif_set_xps_queue() , not i_txqs.

Take an example where num_txqs == 2 and num_rxqs == 2:
total_num_q's == 4, so i will loop until until 4.
When we get to i == 2, it will attempt to map XPS values for
the second TX queue (index 1) but we would be giving the value of
i (which is 2). The index of 2 is greater than the number of available 
tx queues. Somehow __netif_set_xps_queue() is still returning a 
successful error code.
>>
>> Should __netif_set_xps_queue() have a check that index < dev->num_tx_queues?

Re: [PATCH net-next] ibmvnic: Assign XPS map to correct queue index

[Jakub Kicinski]

On Thu, 23 Feb 2023 09:39:44 -0600 Nick Child wrote:
> When setting the XPS map value for TX queues, use the index of the
> transmit queue.
> Previously, the function was passing the index of the loop that iterates
> over all queues (RX and TX). This was causing invalid XPS map values.
> 
> Fixes: 6831582937bd ("ibmvnic: Toggle between queue types in affinity mapping")
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>

Applied, thanks!

> I am a little surprised that __netif_set_xps_queue() did not complain that some
> index values were greater than the number of tx queues. Though maybe the function
> assumes that the developers are wise enough :)
> 
> Should __netif_set_xps_queue() have a check that index < dev->num_tx_queues?

Seems reasonable. Let's wait for the merge window to be over and feel
free to send a patch.