netdev - RE: [PATCH net-next 1/9] lan78xx: add NAPI interface support

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]

Message-ID: <005101d6ffc7$6409d8b0$2c1d8a10$@pebblebay.com>
Date:   Wed, 10 Feb 2021 16:11:09 -0000
From:   "John Efstathiades" <john.efstathiades@...blebay.com>
To:     "'Jesse Brandeburg'" <jesse.brandeburg@...el.com>
Cc:     <UNGLinuxDriver@...rochip.com>, <davem@...emloft.net>,
        <netdev@...r.kernel.org>, <Woojung.Huh@...rochip.com>
Subject: RE: [PATCH net-next 1/9] lan78xx: add NAPI interface support

Apologies for taking a while to respond.

> -----Original Message-----
> From: Jesse Brandeburg <jesse.brandeburg@...el.com>
> Sent: 04 February 2021 20:43
> 
> NB: I thought I'd have a close look at this since I thought I
> understand NAPI pretty well, but using NAPI to transmit frames as well
> as with a usb device has got me pretty confused. 

I'll try to add some more rationale in the next revision of the patch.
However, the short answer is that using NAPI for transmit under high load is
the most effective way of getting the frames into the device's internal
buffer RAM.

> Also, I suspect that
> you didn't try compiling this against the net-next kernel.

I thought I had but it appears not. I won't let that happen again.

> I'm stopping my review only partially completed, please address issues
> https://patchwork.kernel.org/project/netdevbpf/patch/20210204113121.29786-
> 2-john.efstathiades@...blebay.com/

Thanks, will do, but it will take me a few weeks to sort everything out due
to my other commitments.

> It might make it easier for reviewers to split the "infrastructure"
> refactors this patch uses into separate pieces. I know it is more work
> and this is tested already by you, but this is a pretty complicated
> chunk of code to review.

I appreciate this is a complicated patch, a point made by another reviewer. 
Could you explain what you mean by "infrastructure" refactors, please?

I'll certainly look at how to split this patch into smaller chunks but that
might be quite hard to do.

If that turns out not to possible, do you have any suggestions on how I can
make the patch easier for reviewers to understand and review?

John