netdev - Re: [PATCH net 6/7] net: axienet: fix for TX busy handling

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:   Wed, 12 Jan 2022 09:01:28 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Robert Hancock <robert.hancock@...ian.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "radhey.shyam.pandey@...inx.com" <radhey.shyam.pandey@...inx.com>
Subject: Re: [PATCH net 6/7] net: axienet: fix for TX busy handling

On Wed, 12 Jan 2022 16:45:18 +0000 Robert Hancock wrote:
> On Tue, 2022-01-11 at 19:49 -0800, Jakub Kicinski wrote:
> > On Tue, 11 Jan 2022 15:13:57 -0600 Robert Hancock wrote:  
> > > We should be avoiding returning NETDEV_TX_BUSY from ndo_start_xmit in
> > > normal cases. Move the main check for a full TX ring to the end of the
> > > function so that we stop the queue after the last available space is used
> > > up, and only wake up the queue if enough space is available for a full
> > > maximally fragmented packet. Print a warning if there is insufficient
> > > space at the start of start_xmit, since this should no longer happen.
> > > 
> > > Fixes: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI
> > > Ethernet driver")
> > > Signed-off-by: Robert Hancock <robert.hancock@...ian.com>  
> > 
> > Feels a little more like an optimization than strictly a fix.
> > Can we apply this and the following patch to net-next in two
> > week's time? It's not too much of a stretch to take it in now
> > if it's a bit convenience but I don't think the Fixes tags should 
> > stay.  
> 
> Well it's a fix in the sense that it complies with what
> Documentation/networking/driver.rst says drivers should do - I'm not too
> familiar with the consequences of not doing that are, I guess mostly
> performance from having to requeue the packet?

Yes, it's just the re-queuing overhead AFAIU.

> From that standpoint, I guess the concern with breaking those two patches out
> is that the previous patches can introduce a bit of a performance hit (by
> actually caring about the state of the TX ring instead of trampling over it in
> some cases) and so without the last two you might end up with some performance 
> regression. So I'd probably prefer to keep them together with the rest of the
> patch set.

Alright, if you have any numbers on this it'd be great to include them
in the commit message.