Re: [PATCH net-next 3/8] lib: packing: add pack_fields() and unpack_fields()

[Przemek Kitszel]

On 10/11/24 20:48, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
>
> This is new API which caters to the following requirements:
>
> - Pack or unpack a large number of fields to/from a buffer with a small
>    code footprint. The current alternative is to open-code a large number
>    of calls to pack() and unpack(), or to use packing() to reduce that
>    number to half. But packing() is not const-correct.
>
> - Use unpacked numbers stored in variables smaller than u64. This
>    reduces the rodata footprint of the stored field arrays.
>
> - Perform error checking at compile time, rather than at runtime, and
>    return void from the API functions. To that end, we introduce
>    CHECK_PACKED_FIELD_*() macros to be used on the arrays of packed
>    fields. Note: the C preprocessor can't generate variable-length code
>    (loops),  as would be required for array-style definitions of struct
>    packed_field arrays. So the sanity checks use code generation at
>    compile time to $KBUILD_OUTPUT/include/generated/packing-checks.h.
>    There are explicit macros for sanity-checking arrays of 1 packed
>    field, 2 packed fields, 3 packed fields, ..., all the way to 50 packed
>    fields. In practice, the sja1105 driver will actually need the variant
>    with 40 fields. This isn't as bad as it seems: feeding a 39 entry
>    sized array into the CHECK_PACKED_FIELDS_40() macro will actually
>    generate a compilation error, so mistakes are very likely to be caught
>    by the developer and thus are not a problem.
>
> - Reduced rodata footprint for the storage of the packed field arrays.
>    To that end, we have struct packed_field_s (small) and packed_field_m
>    (medium). More can be added as needed (unlikely for now). On these
>    types, the same generic pack_fields() and unpack_fields() API can be
>    used, thanks to the new C11 _Generic() selection feature, which can
>    call pack_fields_s() or pack_fields_m(), depending on the type of the
>    "fields" array - a simplistic form of polymorphism. It is evaluated at
>    compile time which function will actually be called.
>
> Over time, packing() is expected to be completely replaced either with
> pack() or with pack_fields().
>
> Co-developed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
> Signed-off-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> ---
>   include/linux/packing.h  |  69 ++++++++++++++++++++++
>   lib/gen_packing_checks.c |  31 ++++++++++
>   lib/packing.c            | 149 ++++++++++++++++++++++++++++++++++++++++++++++-
>   Kbuild                   |  13 ++++-
>   4 files changed, 259 insertions(+), 3 deletions(-)


> diff --git a/lib/gen_packing_checks.c b/lib/gen_packing_checks.c
> new file mode 100644
> index 000000000000..3213c858c2fe
> --- /dev/null
> +++ b/lib/gen_packing_checks.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +
> +int main(int argc, char **argv)
> +{
> +	printf("/* Automatically generated - do not edit */\n\n");
> +	printf("#ifndef GENERATED_PACKING_CHECKS_H\n");
> +	printf("#define GENERATED_PACKING_CHECKS_H\n\n");
> +
> +	for (int i = 1; i <= 50; i++) {

either you missed my question, or I have missed your reply during
internal round of review, but:

do we need 50? that means 1MB file, while it is 10x smaller for n=27

> +		printf("#define CHECK_PACKED_FIELDS_%d(fields, pbuflen) \\\n", i);
> +		printf("\t({ typeof(&(fields)[0]) _f = (fields); typeof(pbuflen) _len = (pbuflen); \\\n");
> +		printf("\tBUILD_BUG_ON(ARRAY_SIZE(fields) != %d); \\\n", i);
> +		for (int j = 0; j < i; j++) {
> +			int final = (i == 1);

you could replace both @final variables and ternary operators from
the prints by simply moving the final "})\n" outside the loops

> +
> +			printf("\tCHECK_PACKED_FIELD(_f[%d], _len);%s\n",
> +			       j, final ? " })\n" : " \\");
> +		}
> +		for (int j = 1; j < i; j++) {
> +			for (int k = 0; k < j; k++) {
> +				int final = (j == i - 1) && (k == j - 1);
> +
> +				printf("\tCHECK_PACKED_FIELD_OVERLAP(_f[%d], _f[%d]);%s\n",
> +				       k, j, final ? " })\n" : " \\");
> +			}
> +		}
> +	}
> +
> +	printf("#endif /* GENERATED_PACKING_CHECKS_H */\n");
> +}