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

From: Przemek Kitszel
Date: Wed Oct 16 2024 - 09:03:27 EST


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");
+}