netdev - Re: [PATCH net-next 2/2] net: macb: use NAPI for TX completion path

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:   Mon, 9 May 2022 19:46:32 +0000
From:   Robert Hancock <robert.hancock@...ian.com>
To:     "harinik@...inx.com" <harinik@...inx.com>
CC:     "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "tomas.melin@...sala.com" <tomas.melin@...sala.com>,
        "claudiu.beznea@...rochip.com" <claudiu.beznea@...rochip.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "nicolas.ferre@...rochip.com" <nicolas.ferre@...rochip.com>
Subject: Re: [PATCH net-next 2/2] net: macb: use NAPI for TX completion path

On Mon, 2022-05-09 at 14:44 +0530, Harini Katakam wrote:
> Hi Robert,
> 
> Thanks for the patch.
> 
> <snip>
> > Originally I thought there might be a correctness issue with calling it
> > unconditionally, but looking at it further I don't think there really is.
> > The
> > FreeBSD driver for this hardware also seems to always do the TX restart in
> > the
> > interrupt handler if there are packets in the TX queue.
> > 
> > I think the only real issue is whether it's better performance wise to do
> > it
> > all the time rather than only after the hardware asserts a TXUBR interrupt.
> > I
> > expect it would be worse to do it all the time, as that would mean an extra
> > MMIO read, spinlock, MMIO read and MMIO write, versus just a read barrier
> > and
> > checking a flag in memory.
> > 
> 
> I agree that doing TX restart only on UBR is better.
> 
> > > But should there anyways be some condition for the tx side handling, as
> > > I suppose macb_poll() runs when there is rx interrupt even if tx side
> > > has nothing to process?
> > 
> > I opted not to do that for this case, as it should be pretty harmless and
> > cheap
> > to just check the TX ring to see if any packets have been completed yet,
> > rather
> > than actually tracking if a TX completion was pending. That seems to be the
> > standard practice in some other drivers (r8169, etc.)
> 
> In this implementation the TX interrupt bit is being cleared and TX
> processing is
> scheduled when there is an RX interrupt as well as vice versa. Could you
> please
> consider using the check "status & MACB_BIT(TCOMP)" for TX interrupt and
> NAPI?
> If I understand your reply above right, you mention that the above check is
> more
> expensive than parsing the TX ring for new data. In unbalanced traffic
> scenarios i.e.
> server only or client only, will this be efficient?

In the v2 of this patch set I'm about to submit, I'm splitting the NAPI
structures so that TX and RX have their own poll functions, which avoids the
need for any redundant checks of the state of the other ring (TX or RX) when
not triggered by an interrupt.

> 
> Regards,
> Harini