Skip to content

Commit

Permalink
lib: packing: undo loop unrolling of pack_fields_m() and unpack_field…
Browse files Browse the repository at this point in the history 
…s_m()

Jacob Keller came with data in #2
that proves that defining pack_fields_m() and unpack_fields_m() as
macros directly callable by consumer drivers is not a great idea.

We can hide those macros inside the lib/packing.c translation module,
and just provide pack_fields_8(), pack_fields_16(), unpack_fields_8()
and unpack_fields_16() as entry points into the library.

We can even go one step further and expose just pack_fields() and
unpack_fields(), and use the new C11 _Generic() selection feature,
which can call one function or the other, depending on the type of
the "fields" array - a caveman form of polymorphism. It is evaluated at
compile time which function will actually be called.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
  • Loading branch information
@vladimiroltean
vladimiroltean committed Sep 10, 2024
1 parent 28bdf80 commit c77f375
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 109 deletions.
4 changes: 2 additions & 2 deletions drivers/net/dsa/sja1105/sja1105_static_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,9 +500,9 @@ void sja1105_pack(void *buf, u64 val, int start, int end, size_t len);
void sja1105_unpack(const void *buf, u64 *val, int start, int end, size_t len);

#define sja1105_pack_fields(buf, len, ustruct, fields) \
pack_fields_m((buf), (len), (ustruct), (fields), QUIRK_LSW32_IS_FIRST)
pack_fields((buf), (len), (ustruct), (fields), QUIRK_LSW32_IS_FIRST)
#define sja1105_unpack_fields(buf, len, ustruct, fields) \
unpack_fields_m((buf), (len), (ustruct), (fields), QUIRK_LSW32_IS_FIRST)
unpack_fields((buf), (len), (ustruct), (fields), QUIRK_LSW32_IS_FIRST)

/* Common implementations for the static and dynamic configs */
void sja1105pqrs_general_params_entry_pack(void *buf, const void *entry_ptr);
Expand Down
8 changes: 4 additions & 4 deletions drivers/net/ethernet/intel/ice/ice_common.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1417,8 +1417,8 @@ void __ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx, void *buf, size_t len)
CHECK_PACKED_FIELDS_20(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ);
WARN_ON_ONCE(len < ICE_RXQ_CTX_SZ);

pack_fields_m(buf, len, ctx, ice_rlan_ctx_fields,
QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
pack_fields(buf, len, ctx, ice_rlan_ctx_fields,
QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
}

/**
Expand Down Expand Up @@ -1493,8 +1493,8 @@ void __ice_pack_txq_ctx(const struct ice_tlan_ctx *ctx, void *buf, size_t len)
CHECK_PACKED_FIELDS_27(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ);
WARN_ON_ONCE(len < ICE_TXQ_CTX_SZ);

pack_fields_m(buf, len, ctx, ice_tlan_ctx_fields,
QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
pack_fields(buf, len, ctx, ice_tlan_ctx_fields,
QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
}

/* Sideband Queue command wrappers */
Expand Down
86 changes: 25 additions & 61 deletions include/linux/packing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,68 +65,32 @@ struct packed_field {

#include <generated/packing-checks.h>

void pack_fields(void *pbuf, size_t pbuflen, const void *ustruct,
const struct packed_field *fields, size_t num_fields,
u8 quirks);

void unpack_fields(const void *pbuf, size_t pbuflen, void *ustruct,
const struct packed_field *fields, size_t num_fields,
void pack_fields_8(void *pbuf, size_t pbuflen, const void *ustruct,
const struct packed_field_8 *fields, size_t num_fields,
u8 quirks);

#define pack_fields_m(pbuf, pbuflen, ustruct, fields, quirks) \
({ \
size_t num_fields = ARRAY_SIZE(fields); \
\
for (size_t i = 0; i < num_fields; i++) { \
typeof ((fields)[0]) *field = &(fields)[i]; \
u64 uval; \
\
switch (field->size) { \
case 1: \
uval = *((u8 *)((u8 *)ustruct + field->offset)); \
break; \
case 2: \
uval = *((u16 *)((u8 *)ustruct + field->offset)); \
break; \
case 4: \
uval = *((u32 *)((u8 *)ustruct + field->offset)); \
break; \
default: \
uval = *((u64 *)((u8 *)ustruct + field->offset)); \
break; \
} \
\
__pack(pbuf, uval, field->startbit, field->endbit, \
pbuflen, quirks); \
} \
})

#define unpack_fields_m(pbuf, pbuflen, ustruct, fields, quirks) \
({ \
size_t num_fields = ARRAY_SIZE(fields); \
\
for (size_t i = 0; i < num_fields; i++) { \
typeof ((fields)[0]) *field = &fields[i]; \
u64 uval; \
\
__unpack(pbuf, &uval, field->startbit, field->endbit, \
pbuflen, quirks); \
\
switch (field->size) { \
case 1: \
*((u8 *)((u8 *)ustruct + field->offset)) = uval; \
break; \
case 2: \
*((u16 *)((u8 *)ustruct + field->offset)) = uval; \
break; \
case 4: \
*((u32 *)((u8 *)ustruct + field->offset)) = uval; \
break; \
default: \
*((u64 *)((u8 *)ustruct + field->offset)) = uval; \
break; \
} \
} \
})
void pack_fields_16(void *pbuf, size_t pbuflen, const void *ustruct,
const struct packed_field_16 *fields, size_t num_fields,
u8 quirks);

void unpack_fields_8(const void *pbuf, size_t pbuflen, void *ustruct,
const struct packed_field_8 *fields, size_t num_fields,
u8 quirks);

void unpack_fields_16(const void *pbuf, size_t pbuflen, void *ustruct,
const struct packed_field_16 *fields, size_t num_fields,
u8 quirks);

#define pack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
_Generic((fields), \
const struct packed_field_8 *: pack_fields_8, \
default: pack_fields_16 \

This comment has been minimized.

Sign in to view

Copy link
@jacob-keller

jacob-keller Sep 10, 2024

This only supports _8 and _16 right now? And we drop size_t altogether?

Does _Generic support producing an error if we don't specify the right type? I think it would be more readable to specify the types fully and leave out a default.

This comment has been minimized.

Sign in to view

Copy link
@vladimiroltean

vladimiroltean Sep 10, 2024 •
edited
Loading

Author Owner

Apparently yes. "If default is not used and none of the type-names are compatible with the type of the controlling expression, the program will not compile". Well, it won't compile anyway if provided with a random other data type, but due to a different reason than _Generic asserting an expected data type...

This comment has been minimized.

Sign in to view

Copy link
@vladimiroltean

vladimiroltean Sep 10, 2024

Author Owner

This only supports _8 and _16 right now? And we drop size_t altogether?

I think we can drop size_t (struct packed_field) altogether. It isn't used as of now.
)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)

#define unpack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
_Generic((fields), \
const struct packed_field_8 *: unpack_fields_8, \
default: unpack_fields_16 \
)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)

#endif
106 changes: 64 additions & 42 deletions lib/packing.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,32 @@
#include <linux/types.h>
#include <linux/bitrev.h>

#define pack_fields_m(pbuf, pbuflen, ustruct, fields, num_fields, quirks) \
({ \
for (size_t i = 0; i < (num_fields); i++) { \
typeof ((fields)[0]) *field = &(fields)[i]; \
u64 uval; \
\
uval = ustruct_field_to_u64(ustruct, field->offset, field->size); \

This comment has been minimized.

Sign in to view

Copy link
@jacob-keller

jacob-keller Sep 10, 2024

I appreciate that this also avoids the need for the casts by directly using ustring_field_to_u64.
\
__pack(pbuf, uval, field->startbit, field->endbit, \
pbuflen, quirks); \
} \
})

#define unpack_fields_m(pbuf, pbuflen, ustruct, fields, num_fields, quirks) \
({ \
for (size_t i = 0; i < (num_fields); i++) { \
typeof ((fields)[0]) *field = &fields[i]; \
u64 uval; \
\
__unpack(pbuf, &uval, field->startbit, field->endbit, \
pbuflen, quirks); \
\
u64_to_ustruct_field(ustruct, field->offset, field->size, uval); \
} \
})

/**
* calculate_box_addr - Determine physical location of byte in buffer
* @box: Index of byte within buffer seen as a logical big-endian big number
Expand Down Expand Up @@ -130,7 +156,6 @@ void __pack(void *pbuf, u64 uval, size_t startbit, size_t endbit,
((u8 *)pbuf)[box_addr] |= pval;
}
}
EXPORT_SYMBOL(__pack);

/**
* pack - Pack u64 number into bitfield of buffer.
Expand Down Expand Up @@ -243,7 +268,6 @@ void __unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
*uval |= pval;
}
}
EXPORT_SYMBOL(__unpack);

/**
* unpack - Unpack u64 number from packed buffer.
Expand Down Expand Up @@ -287,72 +311,70 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
}
EXPORT_SYMBOL(unpack);

static u64 ustruct_field_to_u64(const void *ustruct, const struct packed_field *field)
static u64 ustruct_field_to_u64(const void *ustruct, size_t field_offset,
size_t field_size)
{
switch (field->size) {
switch (field_size) {
case 1:
return *((u8 *)(ustruct + field->offset));
return *((u8 *)(ustruct + field_offset));
case 2:
return *((u16 *)(ustruct + field->offset));
return *((u16 *)(ustruct + field_offset));
case 4:
return *((u32 *)(ustruct + field->offset));
return *((u32 *)(ustruct + field_offset));
default:
return *((u64 *)(ustruct + field->offset));
return *((u64 *)(ustruct + field_offset));
}
}

static void u64_to_ustruct_field(void *ustruct, const struct packed_field *field,
u64 uval)
static void u64_to_ustruct_field(void *ustruct, size_t field_offset,
size_t field_size, u64 uval)
{
switch (field->size) {
switch (field_size) {
case 1:
*((u8 *)(ustruct + field->offset)) = uval;
*((u8 *)(ustruct + field_offset)) = uval;
break;
case 2:
*((u16 *)(ustruct + field->offset)) = uval;
*((u16 *)(ustruct + field_offset)) = uval;
break;
case 4:
*((u32 *)(ustruct + field->offset)) = uval;
*((u32 *)(ustruct + field_offset)) = uval;
break;
default:
*((u64 *)(ustruct + field->offset)) = uval;
*((u64 *)(ustruct + field_offset)) = uval;
break;
}
}

void pack_fields(void *pbuf, size_t pbuflen, const void *ustruct,
const struct packed_field *fields, size_t num_fields, u8 quirks)
void pack_fields_8(void *pbuf, size_t pbuflen, const void *ustruct,
const struct packed_field_8 *fields, size_t num_fields,
u8 quirks)
{
size_t i;

for (i = 0; i < num_fields; i++) {
const struct packed_field *field = &fields[i];
u64 uval;

uval = ustruct_field_to_u64(ustruct, field);

__pack(pbuf, uval, field->startbit, field->endbit, pbuflen,
quirks);
}
pack_fields_m(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
}
EXPORT_SYMBOL(pack_fields);
EXPORT_SYMBOL(pack_fields_8);

void unpack_fields(const void *pbuf, size_t pbuflen, void *ustruct,
const struct packed_field *fields, size_t num_fields,
u8 quirks)
void pack_fields_16(void *pbuf, size_t pbuflen, const void *ustruct,
const struct packed_field_16 *fields, size_t num_fields,
u8 quirks)
{
size_t i;

for (i = 0; i < num_fields; i++) {
const struct packed_field *field = &fields[i];
u64 uval;
pack_fields_m(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
}
EXPORT_SYMBOL(pack_fields_16);

__unpack(pbuf, &uval, field->startbit, field->endbit, pbuflen,
quirks);
void unpack_fields_8(const void *pbuf, size_t pbuflen, void *ustruct,
const struct packed_field_8 *fields, size_t num_fields,
u8 quirks)
{
unpack_fields_m(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
}
EXPORT_SYMBOL(unpack_fields_8);

u64_to_ustruct_field(ustruct, field, uval);
}
void unpack_fields_16(const void *pbuf, size_t pbuflen, void *ustruct,
const struct packed_field_16 *fields, size_t num_fields,
u8 quirks)
{
unpack_fields_m(pbuf, pbuflen, ustruct, fields, num_fields, quirks);

This comment has been minimized.

Sign in to view

Copy link
@jacob-keller

jacob-keller Sep 10, 2024

Slight duplication, but honestly not that bad.

This comment has been minimized.

Sign in to view

Copy link
@vladimiroltean

vladimiroltean Sep 10, 2024

Author Owner

In fact, duplication is the name of the game here.
Expanding the same macro in 2 scopes with different data types results in different code.
Unless you have a better way of expressing the intention here?
}

This comment has been minimized.

Sign in to view

Copy link
@jacob-keller

jacob-keller Sep 10, 2024

Obviously we don't have any existing users who need the 32 or 64bit sizes for pack fields so I think this makes sense. We can always add those later if/when they become useful.
EXPORT_SYMBOL(unpack_fields);
EXPORT_SYMBOL(unpack_fields_16);

MODULE_DESCRIPTION("Generic bitfield packing and unpacking");

1 comment on commit c77f375

@jacob-keller
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to load this on my live migration branch and test it out to confirm that everything works ok for ice.

Please sign in to comment.