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  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, 16 Oct 2018 17:19:31 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Stephen Hemminger <stephen@...workplumber.org>,
        Heiner Kallweit <hkallweit1@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] r8169: fix NAPI handling under high load



On 10/16/2018 03:17 PM, Stephen Hemminger wrote:
> On Tue, 16 Oct 2018 22:37:31 +0200
> Heiner Kallweit <hkallweit1@...il.com> wrote:
> 
>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>> in the interrupt status register. Under high load NAPI may not be
>> able to process all data (work_done == budget) and it will schedule
>> subsequent calls to the poll callback.
>> rtl_ack_events() however resets the bits in the interrupt status
>> register, therefore subsequent calls to rtl8169_poll() won't call
>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>>
>> Fix this by calling rtl_rx() and rtl_tx() independent of the bits
>> set in the interrupt status register. Both functions will detect
>> if there's nothing to do for them.
>>
>> This issue has been there more or less forever (at least it exists in
>> 3.16 already), so I can't provide a "Fixes" tag. 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> 
> Another issue is this:
> 
> 	if (work_done < budget) {
> 		napi_complete_done(napi, work_done);
> 
> 		rtl_irq_enable(tp, enable_mask);
> 		mmiowb();
> 	}
> 
> 	return work_done;
> }
> 
> 
> The code needs to check return value of napi_complete_done.
> 
> 	if (work_done < budget &&
> 	    napi_complete_done(napi, work_done) {
> 		rtl_irq_enable(tp, enable_mask);
> 		mmiowb();
> 	}
> 
> 	return work_done;
> }
> 
> Try that, it might fix the problem and your logic would
> be unnecessary 
> 

Well, I do not believe this is related.

Testing napi_complete_done() is not mandatory, it is only an optimization [1]
and only for busy polling users.

In short, by default, this should not matter, since busy-polling is not enabled by default.


[1] busy polling users are spinning anyway, so it is not even clear if this
   is really an optimization, unless maybe the cost of irq enabling is really really high...


Powered by blists - more mailing lists