netdev - Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

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]

Date:   Tue, 25 Sep 2018 18:25:49 +0000
From:   Song Liu <songliubraving@...com>
To:     Michael Chan <michael.chan@...adcom.com>
CC:     Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Ariel Elior <ariel.elior@...ium.com>,
        "Eric Dumazet" <eric.dumazet@...il.com>,
        "tariqt@...lanox.com" <tariqt@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        "jeffrey.t.kirsher@...el.com" <jeffrey.t.kirsher@...el.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        "j.vosburgh@...il.com" <j.vosburgh@...il.com>,
        "vfalico@...il.com" <vfalico@...il.com>,
        "Andy Gospodarek" <andy@...yhouse.net>
Subject: Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers



> On Sep 25, 2018, at 7:43 AM, Michael Chan <michael.chan@...adcom.com> wrote:
> 
> On Tue, Sep 25, 2018 at 7:20 AM Eric Dumazet <edumazet@...gle.com> wrote:
>> 
>> On Tue, Sep 25, 2018 at 7:02 AM Michael Chan <michael.chan@...adcom.com> wrote:
>>> 
>>> On Mon, Sep 24, 2018 at 2:18 PM Song Liu <songliubraving@...com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@...gle.com> wrote:
>>>>> 
>>>>>> 
>>>>>> Interesting, maybe a bnxt specific issue.
>>>>>> 
>>>>>> It seems their model is to process TX/RX notification in the same queue,
>>>>>> they throw away RX events if budget == 0
>>>>>> 
>>>>>> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
>>>>>> must be reverted.
>>>>>> 
>>>>>> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
>>>>> 
>>>>> Hmm, actually a revert might not be enough, since code at lines 2030-2031
>>>>> would fire and we might not call napi_complete_done() anyway.
>>>>> 
>>>>> Unfortunately this driver logic is quite complex.
>>>>> 
>>>>> Could you test on other NIC eventually ?
>>>>> 
>>>> 
>>>> It actually runs OK on ixgbe.
>>>> 
>>>> @Michael, could you please help us with this?
>>>> 
>>> I've taken a quick look using today's net tree plus Eric's
>>> poll_one_napi() patch.  The problem I'm seeing is that netpoll calls
>>> bnxt_poll() with budget 0.  And since work_done >= budget of 0, we
>>> return without calling napi_complete_done() and without arming the
>>> interrupt.  netpoll doesn't always call us back until we call
>>> napi_complete_done(), right?  So I think if there are in-flight TX
>>> completions, we'll miss those.
>> 
>> That's the whole point of netpoll :
>> 
>> We drain the TX queues, without interrupts being involved at all,
>> by calling ->napi() with a zero budget.
>> 
>> napi_complete(), even if called from ->napi() while budget was zero,
>> should do nothing but return early.
>> 
>> budget==0 means that ->napi() should process all TX completions.
> 
> All TX completions that we can see.  We cannot see the in-flight ones.
> 
> If budget is exceeded, I think the assumption is that poll will always
> be called again.
> 
>> 
>> So it looks like bnxt has a bug, that is showing up after the latest
>> poll_one_napi() patch.
>> This latest patch is needed otherwise the cpu attempting the
>> netpoll-TX-drain might drain nothing at all,
>> since it does not anymore call ndo_poll_controller() that was grabbing
>> SCHED bits on all queues (napi_schedule() like calls)
> 
> I think the latest patch is preventing the normal interrupt -> NAPI
> path from coming in and cleaning the remaining TX completions and
> arming the interrupt.

Hi Michael, 

This may not be related. But I am looking at this:

bnxt_poll_work() {

	while (1) {
		....
		if (rx_pkts == budget)
			return
	}
}

With budget of 0, the loop will terminate after processing one packet. 
But I think the expectation is to finish all tx packets. So it doesn't
feel right. Could you please confirm?

Thanks,
Song