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

Date:   Mon, 9 May 2022 14:44:22 +0530
From:   Harini Katakam <harinik@...inx.com>
To:     Robert Hancock <robert.hancock@...ian.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "tomas.melin@...sala.com" <tomas.melin@...sala.com>,
        "nicolas.ferre@...rochip.com" <nicolas.ferre@...rochip.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "claudiu.beznea@...rochip.com" <claudiu.beznea@...rochip.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>
Subject: Re: [PATCH net-next 2/2] net: macb: use NAPI for TX completion path

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?

Regards,
Harini