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

Jacob Keller
2024-10-15T19:19:16Z

On 10/11/2024 11:48 AM, 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/include/linux/packing.h b/include/linux/packing.h
> index 5d36dcd06f60..eeb23d90e5e0 100644
> --- a/include/linux/packing.h
> +++ b/include/linux/packing.h
> @@ -26,4 +26,73 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
> int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
> size_t pbuflen, u8 quirks);
>
> +#define GEN_PACKED_FIELD_MEMBERS(__type) \
> + __type startbit; \
> + __type endbit; \
> + __type offset; \
> + __type size;
> +
> +/* Small packed field. Use with bit offsets < 256, buffers < 32B and
> + * unpacked structures < 256B.
> + */
> +struct packed_field_s {
> + GEN_PACKED_FIELD_MEMBERS(u8);
> +};
> +
> +/* Medium packed field. Use with bit offsets < 65536, buffers < 8KB and
> + * unpacked structures < 64KB.
> + */
> +struct packed_field_m {
> + GEN_PACKED_FIELD_MEMBERS(u16);
> +};
> +
> +#define PACKED_FIELD(start, end, struct_name, struct_field) \
> + { \
> + (start), \
> + (end), \
> + offsetof(struct_name, struct_field), \
> + sizeof_field(struct_name, struct_field), \
> + }
> +
> +#define CHECK_PACKED_FIELD(field, pbuflen) \
> + ({ typeof(field) __f = (field); typeof(pbuflen) __len = (pbuflen); \
> + BUILD_BUG_ON(__f.startbit < __f.endbit); \
> + BUILD_BUG_ON(__f.startbit >= BITS_PER_BYTE * __len); \
> + BUILD_BUG_ON(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size); \
> + BUILD_BUG_ON(__f.size != 1 && __f.size != 2 && __f.size != 4 && __f.size != 8); })
> +
> +#define CHECK_PACKED_FIELD_OVERLAP(field1, field2) \
> + ({ typeof(field1) _f1 = (field1); typeof(field2) _f2 = (field2); \
> + BUILD_BUG_ON(max(_f1.endbit, _f2.endbit) <= min(_f1.startbit, _f2.startbit)); })
> +
> +#include <generated/packing-checks.h>
> +
> +void pack_fields_s(void *pbuf, size_t pbuflen, const void *ustruct,
> + const struct packed_field_s *fields, size_t num_fields,
> + u8 quirks);
> +
> +void pack_fields_m(void *pbuf, size_t pbuflen, const void *ustruct,
> + const struct packed_field_m *fields, size_t num_fields,
> + u8 quirks);
> +
> +void unpack_fields_s(const void *pbuf, size_t pbuflen, void *ustruct,
> + const struct packed_field_s *fields, size_t num_fields,
> + u8 quirks);
> +
> +void unpack_fields_m(const void *pbuf, size_t pbuflen, void *ustruct,
> + const struct packed_field_m *fields, size_t num_fields,
> + u8 quirks);
> +
> +#define pack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
> + _Generic((fields), \
> + const struct packed_field_s * : pack_fields_s, \
> + const struct packed_field_m * : pack_fields_m \
> + )(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)
> +
> +#define unpack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
> + _Generic((fields), \
> + const struct packed_field_s * : unpack_fields_s, \
> + const struct packed_field_m * : unpack_fields_m \
> + )(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)
> +
> #endif
> 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++) {
> + 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);
> +
> + 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");
> +}

Hi Masahiro,

The changes in this patch contains some code and Kbuild changes to
generate compile-time macro checks at build time (instead of committing
20 thousand lines of code directly to git). I'd appreciate if you could
review this change, specifically the auto-generation of packing-checks.h

The full series can be viewed on lore at:

> https://lore.kernel.org/netdev/20241011-packing-pack-fields-and-ice-implementation-v1-0-d9b1f7500740@intel.com/

> diff --git a/Kbuild b/Kbuild
> index 464b34a08f51..35a8b78b72d9 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -34,6 +34,17 @@ arch/$(SRCARCH)/kernel/asm-offsets.s: $(timeconst-file) $(bounds-file)
> $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
> $(call filechk,offsets,__ASM_OFFSETS_H__)
>
> +# Generate packing-checks.h
> +
> +hostprogs += lib/gen_packing_checks
> +
> +packing-checks := include/generated/packing-checks.h
> +
> +filechk_gen_packing_checks = lib/gen_packing_checks
> +
> +$(packing-checks): lib/gen_packing_checks FORCE
> + $(call filechk,gen_packing_checks)
> +
> # Check for missing system calls
>
> quiet_cmd_syscalls = CALL $<
> @@ -70,7 +81,7 @@ $(atomic-checks): $(obj)/.checked-%: include/linux/atomic/% FORCE
> # A phony target that depends on all the preparation targets
>
> PHONY += prepare
> -prepare: $(offsets-file) missing-syscalls $(atomic-checks)
> +prepare: $(offsets-file) missing-syscalls $(atomic-checks) $(packing-checks)
> @:
>
> # Ordinary directory descending
>

In particular, I tried a variety of places to put the build-time
generation logic, and ended up having to stick it in the top level
Kbuild file as part of the prepare target. I was unable to figure out
another way to get the include dependency correct.

packing-checks.h contains the set of macros generated for checking
various sizes of the packing array, which we want to have at compile
time, so we need to generate packing-checks.h before any code which
includes <linux/packing.h> which is what ultimately includes
<generated/packing-checks.h>

Vladimir and I tried to come up with other methods of doing the compile
time checking and validation of the structures. But the limited C
pre-processor prevents us from doing loops, which is what led to the
large number of generated macros.

Thanks,
Jake