netdev - Re: pegged softirq and NAPI race (?)

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ADCEA4AC-0071-47B7-A539-5F219EF09206@fb.com>
Date:   Tue, 18 Sep 2018 21:40:56 +0000
From:   Song Liu <songliubraving@...com>
To:     "jeffrey.t.kirsher@...el.com" <jeffrey.t.kirsher@...el.com>
CC:     Eric Dumazet <eric.dumazet@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Alexei Starovoitov <ast@...com>,
        netdev <netdev@...r.kernel.org>,
        "Alexander Duyck" <alexander.h.duyck@...el.com>,
        "michael.chan@...adcom.com" <michael.chan@...adcom.com>,
        Kernel Team <Kernel-team@...com>
Subject: Re: pegged softirq and NAPI race (?)



> On Sep 18, 2018, at 2:36 PM, Jeff Kirsher <jeffrey.t.kirsher@...el.com> wrote:
> 
> On Tue, 2018-09-18 at 14:21 -0700, Eric Dumazet wrote:
>> 
>> On 09/18/2018 02:13 PM, Eric Dumazet wrote:
>>> On Tue, Sep 18, 2018 at 1:37 PM Song Liu <songliubraving@...com>
>>> wrote:
>>>> 
>>>> Looks like a patch like the following fixes the issue for ixgbe.
>>>> But I
>>>> cannot explain it yet.
>>>> 
>>>> Does this ring a bell?
>>> 
>>> I dunno, it looks like the NIC is  generating an interrupt while it
>>> should not,
>>> and constantly sets NAPI_STATE_MISSED.
>>> 
>>> Or maybe we need to properly check napi_complete_done() return
>>> value.
>>> 
>>> diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> index
>>> d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedc
>>> dcb72dc1c401813
>>> 100644
>>> --- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> @@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int
>>> budget)
>>>        ixgb_clean_rx_irq(adapter, &work_done, budget);
>>> 
>>>        /* If budget not fully consumed, exit the polling mode */
>>> -       if (work_done < budget) {
>>> -               napi_complete_done(napi, work_done);
>>> +       if (work_done < budget &&
>>> +           napi_complete_done(napi, work_done)) {
>>>                if (!test_bit(__IXGB_DOWN, &adapter->flags))
>>>                        ixgb_irq_enable(adapter);
>>>        }
>>> 
>> 
>> 
>> ixgbe patch would be :
>> 
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index
>> 604282f03d236e4358fc91e64d8ba00a9b36cb8c..80d00aecb6e3e3e950ce6309bfe
>> 3639953dd73d9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3196,12 +3196,12 @@ int ixgbe_poll(struct napi_struct *napi, int
>> budget)
>>                return budget;
>> 
>>        /* all work done, exit the polling mode */
>> -       napi_complete_done(napi, work_done);
>> -       if (adapter->rx_itr_setting & 1)
>> -               ixgbe_set_itr(q_vector);
>> -       if (!test_bit(__IXGBE_DOWN, &adapter->state))
>> -               ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector-
>>> v_idx));
>> -
>> +       if (napi_complete_done(napi, work_done)) {

Shall we add likely() here? 

>> +               if (adapter->rx_itr_setting & 1)
>> +                       ixgbe_set_itr(q_vector);
>> +               if (!test_bit(__IXGBE_DOWN, &adapter->state))
>> +                       ixgbe_irq_enable_queues(adapter,
>> BIT_ULL(q_vector->v_idx));
>> +       }
>>        return min(work_done, budget - 1);
>> }
>> 
> 
> Eric, after Song does some testing on these changes, will you be
> submitting a formal patch?  If so, make sure to send it to 
> intel-wired-lan@...ts.osuosl.org mailing list so I can pick up the fix.
> 
> By the way, thanks!

I would submit the patch if Eric prefer not to. :)

Thanks,
Song