Message-ID: <20241211202106.ic2nlfj7ep3j4f3s@skbuf>
Date: Wed, 11 Dec 2024 22:21:06 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Masahiro Yamada <masahiroy@...nel.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v10 00/10] lib: packing: introduce and use
 (un)pack_fields

On Tue, Dec 10, 2024 at 12:27:09PM -0800, Jacob Keller wrote:
> This series improves the packing library with a new API for packing or
> unpacking a large number of fields at once with minimal code footprint. The
> API is then used to replace bespoke packing logic in the ice driver,
> preparing it to handle unpacking in the future. Finally, the ice driver has
> a few other cleanups related to the packing logic.
> 
> The pack_fields and unpack_fields functions have the following improvements
> over the existing pack() and unpack() API:
> 
>  1. Packing or unpacking a large number of fields takes significantly less
>     code. This significantly reduces the .text size for an increase in the
>     .data size which is much smaller.
> 
>  2. The unpacked data can be stored in sizes smaller than u64 variables.
>     This reduces the storage requirement both for runtime data structures,
>     and for the rodata defining the fields. This scales with the number of
>     fields used.
> 
>  3. Most of the error checking is done at compile time, rather than
>     runtime, via CHECK_PACKED_FIELD macros.
> 
> The actual packing and unpacking code still uses the u64 size
> variables. However, these are converted to the appropriate field sizes when
> storing or reading the data from the buffer.
> 
> This version now uses significantly improved macro checks, thanks to the
> work of Vladimir. We now only need 300 lines of macro for the generated
> checks. In addition, each new check only requires 4 lines of code for its
> macro implementation and 1 extra line in the CHECK_PACKED_FIELDS macro.
> This is significantly better than previous versions which required ~2700
> lines.
> 
> The CHECK_PACKED_FIELDS macro uses __builtin_choose_expr to select the
> appropriately sized CHECK_PACKED_FIELDS_N macro. This enables directly
> adding CHECK_PACKED_FIELDS calls into the pack_fields and unpack_fields
> macros. Drivers no longer need to call the CHECK_PACKED_FIELDS_N macros
> directly, and we do not need to modify Kbuild or introduce multiple CONFIG
> options.
> 
> The code for the CHECK_PACKED_FIELDS_(0..50) and CHECK_PACKED_FIELDS itself
> can be generated from the C program in scripts/gen_packed_field_checks.c.
> This little C program may be used in the future to update the checks to
> more sizes if a driver with more than 50 fields appears in the future.
> The total amount of required code is now much smaller, and we don't
> anticipate needing to increase the size very often. Thus, it makes sense to
> simply commit the result directly instead of attempting to modify Kbuild to
> automatically generate it.
> 
> This version uses the 5-argument format of pack_fields and unpack_fields,
> with the size of the packed buffer passed as one of the arguments. We do
> enforce that the compiler can tell its a constant using
> __builtin_constant_p(), ensuring that the size checks are handled at
> compile time. We could reduce these to 4 arguments and require that the
> passed in pbuf be of a type which has the appropriate size. I opted against
> that because it makes the API less flexible and a bit less natural to use
> in existing code.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> ---

Any reason why you aren't carrying over my review and test tags from one
version to another?

Reviewed-by: Vladimir Oltean <vladimir.oltean@....com>

Powered by blists - more mailing lists