Re: [PATCH net-next v10 00/10] lib: packing: introduce and use (un)pack_fields

[Jacob Keller <jacob.e.keller@intel.com>]

On 12/11/2024 12:21 PM, Vladimir Oltean wrote:
> 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@intel.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@nxp.com>

Sorry about that, I forgot my usual step of running b4 trailers.