Re: [PATCH net-next 3/8] lib: packing: add pack_fields() and unpack_fields()
From: Jacob Keller <jacob.e.keller@intel.com>
Date: 2024-10-15 19:19:16
Also in:
lkml
On 10/11/2024 11:48 AM, Jacob Keller wrote:
quoted hunk ↗ jump to hunk
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) + #endifdiff --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/ (local)
quoted hunk ↗ jump to hunk
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