netdev - Re: mlx4: Bug in XDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 21 Dec 2016 18:47:28 +0200
From:   Tariq Toukan <ttoukan.linux@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     Tariq Toukan <tariqt@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>
Subject: Re: mlx4: Bug in XDP_TX + 16 rx-queues



On 20/12/2016 8:31 PM, Martin KaFai Lau wrote:
> On Tue, Dec 20, 2016 at 02:02:05PM +0200, Tariq Toukan wrote:
>> Thanks Martin, nice catch!
>>
>>
>> On 20/12/2016 1:37 AM, Martin KaFai Lau wrote:
>>> Hi Tariq,
>>>
>>> On Sat, Dec 17, 2016 at 02:18:03AM -0800, Martin KaFai Lau wrote:
>>>> Hi All,
>>>>
>>>> I have been debugging with XDP_TX and 16 rx-queues.
>>>>
>>>> 1) When 16 rx-queues is used and an XDP prog is doing XDP_TX,
>>>> it seems that the packet cannot be XDP_TX out if the pkt
>>>> is received from some particular CPUs (/rx-queues).
>>>>
>>>> 2) If 8 rx-queues is used, it does not have problem.
>>>>
>>>> 3) The 16 rx-queues problem also went away after reverting these
>>>> two patches:
>>>> 15fca2c8eb41 net/mlx4_en: Add ethtool statistics for XDP cases
>>>> 67f8b1dcb9ee net/mlx4_en: Refactor the XDP forwarding rings scheme
>>>>
>>> After taking a closer look at 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme")
>>> and armed with the fact that '>8 rx-queues does not work', I have
>>> made the attached change that fixed the issue.
>>>
>>> Making change in mlx4_en_fill_qp_context() could be an easier fix
>>> but I think this change will be easier for discussion purpose.
>>>
>>> I don't want to lie that I know anything about how this variable
>>> works in CX3.  If this change makes sense, I can cook up a diff.
>>> Otherwise, can you shed some light on what could be happening
>>> and hopefully can lead to a diff?
>>>
>>> Thanks
>>> --Martin
>>>
>>>
>>> diff --git i/drivers/net/ethernet/mellanox/mlx4/en_netdev.c w/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>>> index bcd955339058..b3bfb987e493 100644
>>> --- i/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>>> +++ w/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>>> @@ -1638,10 +1638,10 @@ int mlx4_en_start_port(struct net_device *dev)
>>>
>>>   	/* Configure tx cq's and rings */
>>>   	for (t = 0 ; t < MLX4_EN_NUM_TX_TYPES; t++) {
>>> -		u8 num_tx_rings_p_up = t == TX ? priv->num_tx_rings_p_up : 1;
>> The bug lies in this line.
>> Number of rings per UP in case of TX_XDP should be priv->tx_ring_num[TX_XDP
>> ], not 1.
>> Please try the following fix.
>> I can prepare and send it once the window opens again.
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> index bcd955339058..edbe200ac2fa 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> @@ -1638,7 +1638,8 @@ int mlx4_en_start_port(struct net_device *dev)
>>
>>          /* Configure tx cq's and rings */
>>          for (t = 0 ; t < MLX4_EN_NUM_TX_TYPES; t++) {
>> -               u8 num_tx_rings_p_up = t == TX ? priv->num_tx_rings_p_up :
>> 1;
>> +               u8 num_tx_rings_p_up = t == TX ?
>> +                       priv->num_tx_rings_p_up : priv->tx_ring_num[t];
>>
>>                  for (i = 0; i < priv->tx_ring_num[t]; i++) {
>>                          /* Configure cq */
>>
> Thanks for confirming the bug is related to the user_prio argument.
>
> I have some questions:
>
> 1. Just to confirm the intention of the change.  Your change is essentially
>     always passing 0 to the user_prio parameter for the TX_XDP type by
>     doing (i / priv->tx_ring_num[t])?
Yes
>    If yes, would it be clearer to
>     always pass 0 instead?
I think that it should be implied by num_tx_rings_p_up, it simply hadthe 
wrong value.
>
>     And yes, it also works in our test.  Please post an offical patch
>     if it is the fix.
Sure.
>
> 2. Can you explain a little on how does the user_prio affect
>     the tx behavior?  e.g. What is the difference between
>     different user_prio like 0, 1, 2...etc?
An implementation of QoS (Quality of Service).
This would tell the HW to prioritize between different send queues.
In XDP forward we use a single user prio, 0 (default).
>
> 3. Mostly a follow up on (2).
>     In mlx4_en_get_profile(), num_tx_rings_p_up (of the struct mlx4_en_profile)
>     depends on mlx4_low_memory_profile() and number of cpu.  Does these
>     similar bounds apply to the 'u8 num_tx_rings_p_up' here for
>     TX_XDP type?
No.
The number of XDP_TX rings is equal to the number of RX rings.
>
> Thanks,
> Martin
Regards,
Tariq
>>> -
>>>   		for (i = 0; i < priv->tx_ring_num[t]; i++) {
>>>   			/* Configure cq */
>>> +			int user_prio;
>>> +
>>>   			cq = priv->tx_cq[t][i];
>>>   			err = mlx4_en_activate_cq(priv, cq, i);
>>>   			if (err) {
>>> @@ -1660,9 +1660,14 @@ int mlx4_en_start_port(struct net_device *dev)
>>>
>>>   			/* Configure ring */
>>>   			tx_ring = priv->tx_ring[t][i];
>>> +			if (t != TX_XDP)
>>> +				user_prio = i / priv->num_tx_rings_p_up;
>>> +			else
>>> +				user_prio = i & 0x07;
>>> +
>>>   			err = mlx4_en_activate_tx_ring(priv, tx_ring,
>>>   						       cq->mcq.cqn,
>>> -						       i / num_tx_rings_p_up);
>>> +						       user_prio);
>>>   			if (err) {
>>>   				en_err(priv, "Failed allocating Tx ring\n");
>>>   				mlx4_en_deactivate_cq(priv, cq);
>> Regards,
>> Tariq Toukan.