netdev - Re: [net-next 3/6] ravb: Create helper to allocate skb and align it

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]

Message-ID: <88c93368-6dc7-45ea-9bae-e1cbc50d69a7@bp.renesas.com>
Date: Tue, 27 Feb 2024 10:21:09 +0000
From: Paul Barker <paul.barker.ct@...renesas.com>
To: Niklas Söderlund
 <niklas.soderlund+renesas@...natech.se>, Sergey Shtylyov
 <s.shtylyov@....ru>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Biju Das <biju.das.jz@...renesas.com>,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
 Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>, netdev@...r.kernel.org
Cc: linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next 3/6] ravb: Create helper to allocate skb and align it

On 27/02/2024 01:40, Niklas Söderlund wrote:
> The RAVB device requires the SKB data to be aligned to 128 bytes. The
> alignment is done by allocating a skb 128 bytes larger than the maximum
> frame size supported by the device and adjusting the headroom to fit the
> requirement.
> 
> This code has been refactored a few times and small issues have been
> added along the way. The issues are not harmful but prevents merging
> parts of the Rx code which have been split in two implementations with
> the addition of RZ/G2L support, a device that supports larger frame
> sizes.
> 
> This change removes the need for duplicated and somewhat inaccurate
> hardware alignment constrains stored in the hardware information struct
> by creating a helper to handle the allocation of a skb and alignment of
> a skb data.
> 
> For the R-Car class of devices the maximum frame size is 4K and each
> descriptor is limited to 2K of data. The current implementation does not
> support split descriptors, this limits the frame size to 2K. The
> current hardware information however records the descriptor size just
> under 2K due to bad understanding of the device when larger MTUs where
> added.
> 
> For the RZ/G2L device the maximum frame size is 8K and each descriptor
> is limited to 4K of data. The current hardware information records this
> correctly, but it gets the alignment constrains wrong as just aligns it
> by 128, it does not extend it by 128 bytes to allow the full frame to be
> stored. This works because the RZ/G2L device supports split descriptors
> and allocates each skb to 8K and aligns each 4K descriptor in this
> space.

This change tidies up code which I re-wrote last week but I haven't sent
patches yet... I was holding off sending further patches until we've
completed gPTP testing, but maybe I can get an updated RFC series sent
first then continue with testing.

To get the best RX performance, we should allocate plain RX buffers
using a page pool, then call build_skb() when a packet is received.
Reducing RX buffer size from 8kB to 2kB for RZ/G2L gets me a 50%
improvement in TCP RX throughput and a 200% improvement in UDP RX
throughput. Using a page pool gets me a further 10% or so in both TCP &
UDP RX throughput. So, if we merge this I'll be trying to remove
ravb_alloc_skb() shortly anyway.

So NACK on this for now, but let's co-ordinate the improvements we're
both making so there aren't any merge conflicts for the maintainers to
resolve.

Thanks,

-- 
Paul Barker
Download attachment "OpenPGP_0x27F4B3459F002257.asc" of type "application/pgp-keys" (3521 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)