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

Przemek Kitszel
2024-10-16T13:02:47Z

On 10/11/24 20:48, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> 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@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> 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");
> +}