Message-ID: <56a3653a-bcec-4755-bafc-03bf1e7cee3a@intel.com>
Date: Wed, 4 Dec 2024 10:47:24 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Andrew Morton <akpm@...ux-foundation.org>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Przemek
 Kitszel" <przemyslaw.kitszel@...el.com>, Masahiro Yamada
	<masahiroy@...nel.org>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v8 03/10] lib: packing: add pack_fields() and
 unpack_fields()



On 12/4/2024 9:12 AM, Vladimir Oltean wrote:
> On Tue, Dec 03, 2024 at 03:53:49PM -0800, Jacob Keller wrote:
>> +#define CHECK_PACKED_FIELD(field) ({ \
>> +	typeof(field) __f = (field); \
>> +	BUILD_BUG_ON(__f.startbit < __f.endbit); \
>> +	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(ascending, field1, field2) ({ \
>> +	typeof(field1) _f1 = (field1); typeof(field2) _f2 = (field2); \
>> +	const bool _a = (ascending); \
>> +	BUILD_BUG_ON(_a && _f1.startbit >= _f2.startbit); \
>> +	BUILD_BUG_ON(!_a && _f1.startbit <= _f2.startbit); \
>> +	BUILD_BUG_ON(max(_f1.endbit, _f2.endbit) <= \
>> +		     min(_f1.startbit, _f2.startbit)); \
>> +})
>> +
>> +#define CHECK_PACKED_FIELDS_SIZE(fields, pbuflen) ({ \
>> +	typeof(&(fields)[0]) _f = (fields); \
>> +	typeof(pbuflen) _len = (pbuflen); \
>> +	const size_t num_fields = ARRAY_SIZE(fields); \
>> +	BUILD_BUG_ON(!__builtin_constant_p(_len)); \
>> +	BUILD_BUG_ON(_f[0].startbit >= BITS_PER_BYTE * _len); \
> 
> Please add a comment here stating that we check both the first and last
> element to cover the ascending as well as descending ordering scenarios.
> It took me a while to realize this, I thought the _f[0] check was unnecessary.
> 

Sure. I will also try to adopt the BUILD_BUG_ON_MSG style you have used
for the overlap to help with better error messages.

>> +	BUILD_BUG_ON(_f[num_fields - 1].startbit >= BITS_PER_BYTE * _len); \
>> +})
>> +
>>  #define QUIRK_MSB_ON_THE_RIGHT	BIT(0)
>>  #define QUIRK_LITTLE_ENDIAN	BIT(1)
>>  #define QUIRK_LSW32_IS_FIRST	BIT(2)
> 
> I spent some time today to play around with this version, and it seems
> to work, but I took some liberty and made the following changes:
> 
> - Tail-call CHECK_PACKED_FIELD_OVERLAP() from CHECK_PACKED_FIELD(). This
>   reduces the size of the generated code from 2753 lines to 1478 lines,
>   which already brings it a little bit more into the realm of "tolerable" IMO.
> 

Nice thats a good improvement.

> - Remove the BUILD_BUG_ON(ARRAY_SIZE(fields) == N), since I think
>   that's just wasteful (in terms of space and compiler CPU cycles) and
>   ultra-defensive, when the auto-generated __builtin_choose_expr() is
>   the only caller. It was justified when the consumer had to explicitly
>   select the right checking macro.
> 

Sure, we can drop these. I had kept them because I felt it was
worthwhile in case someone does call them manually, but I don't think we
should encourage that behavior, and it is otherwise completely wasted
cycles. I guess its defense against screwing up the builtin_choose_expr,
but that is auto-generated now too.

> - Add some prettier error messages. Compare (for an error injected by me):
> 
> ../drivers/net/ethernet/intel/ice/ice_common.c:1419:2: error: call to '__compiletime_assert_3302' declared with 'error' attribute: BUILD_BUG_ON failed: max(_f1.endbit, _f2.endbit) <= min(_f1.startbit, _f2.startbit)
>         pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
>         ^
> 
> with:
> 
> 
> ../drivers/net/ethernet/intel/ice/ice_common.c:1419:2: error: call to '__compiletime_assert_3414' declared with 'error' attribute: ice_rlan_ctx_fields field 3 overlaps with previous field
>         pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
>         ^
> 

That is much nicer, thanks.

> 
> That incremental improvement is below, if you'd be interested in including it
> (the auto-generated code is not part of the diff):
> 
> diff --git a/include/linux/packing.h b/include/linux/packing.h
> index c4fc76ae64a5..1c89a5129b06 100644
> --- a/include/linux/packing.h
> +++ b/include/linux/packing.h
> @@ -36,22 +36,38 @@ struct packed_field_m {
>  	sizeof_field(struct_name, struct_field), \
>  }
>  
> -#define CHECK_PACKED_FIELD(field) ({ \
> -	typeof(field) __f = (field); \
> -	BUILD_BUG_ON(__f.startbit < __f.endbit); \
> -	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(fields, index1, index2) ({ \
> +	typeof(&(fields)[0]) __f = (fields); \
> +	typeof(__f[0]) _f1 = __f[index1]; typeof(__f[0]) _f2 = __f[index2]; \
> +	const bool _ascending = __f[0].startbit < __f[1].startbit; \
> +	BUILD_BUG_ON_MSG(_ascending && _f1.startbit >= _f2.startbit, \
> +			 __stringify(fields) " field " __stringify(index2) \
> +			 " breaks ascending order"); \
> +	BUILD_BUG_ON_MSG(!_ascending && _f1.startbit <= _f2.startbit, \
> +			 __stringify(fields) " field " __stringify(index2) \
> +			 " breaks descending order"); \
> +	BUILD_BUG_ON_MSG(max(_f1.endbit, _f2.endbit) <= \
> +			 min(_f1.startbit, _f2.startbit), \
> +			 __stringify(fields) " field " __stringify(index2) \
> +			 " overlaps with previous field"); \
>  })
>  
> -
> -#define CHECK_PACKED_FIELD_OVERLAP(ascending, field1, field2) ({ \
> -	typeof(field1) _f1 = (field1); typeof(field2) _f2 = (field2); \
> -	const bool _a = (ascending); \
> -	BUILD_BUG_ON(_a && _f1.startbit >= _f2.startbit); \
> -	BUILD_BUG_ON(!_a && _f1.startbit <= _f2.startbit); \
> -	BUILD_BUG_ON(max(_f1.endbit, _f2.endbit) <= \
> -		     min(_f1.startbit, _f2.startbit)); \
> +#define CHECK_PACKED_FIELD(fields, index) ({ \
> +	typeof(&(fields)[0]) _f = (fields); \
> +	typeof(_f[0]) __f = _f[index]; \
> +	BUILD_BUG_ON_MSG(__f.startbit < __f.endbit, \
> +			 __stringify(fields) " field " __stringify(index) \
> +			 " start bit must not be smaller than end bit"); \
> +	BUILD_BUG_ON_MSG(__f.size != 1 && __f.size != 2 && \
> +			 __f.size != 4 && __f.size != 8, \
> +			 __stringify(fields) " field " __stringify(index) \
> +			" has unsupported unpacked storage size"); \
> +	BUILD_BUG_ON_MSG(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size, \
> +			 __stringify(fields) " field " __stringify(index) \
> +			 " exceeds unpacked storage size"); \
> +	__builtin_choose_expr(index != 0, \
> +			      CHECK_PACKED_FIELD_OVERLAP(fields, index - 1, index), \
> +			      1); \
>  })
>  
>  #define CHECK_PACKED_FIELDS_SIZE(fields, pbuflen) ({ \
> diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c
> index 09a21afd640b..fabbb741c9a8 100644
> --- a/scripts/gen_packed_field_checks.c
> +++ b/scripts/gen_packed_field_checks.c
> @@ -9,15 +9,9 @@ int main(int argc, char **argv)
>  {
>  	for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) {
>  		printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
> -		printf("\ttypeof(&(fields)[0]) _f = (fields); \\\n");
> -		printf("\tBUILD_BUG_ON(ARRAY_SIZE(fields) != %d); \\\n", i);
>  
>  		for (int j = 0; j < i; j++)
> -			printf("\tCHECK_PACKED_FIELD(_f[%d]); \\\n", j);
> -
> -		for (int j = 1; j < i; j++)
> -			printf("\tCHECK_PACKED_FIELD_OVERLAP(_f[0].startbit < _f[1].startbit, _f[%d], _f[%d]); \\\n",
> -			       j - 1, j);
> +			printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);
>  
>  		printf("})\n\n");
>  	}
> 
> And there's one more thing I tried, which mostly worked. That was to
> express CHECK_PACKED_FIELDS_N in terms of CHECK_PACKED_FIELDS_N-1.
> This further reduced the auto-generated code size from 1478 lines to 302
> lines, which I think is appealing.
> 
> diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c
> index fabbb741c9a8..bac85c04ef20 100644
> --- a/scripts/gen_packed_field_checks.c
> +++ b/scripts/gen_packed_field_checks.c
> @@ -10,9 +10,10 @@ int main(int argc, char **argv)
>  	for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) {
>  		printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
>  
> -		for (int j = 0; j < i; j++)
> -			printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);
> +		if (i != 1)
> +			printf("\tCHECK_PACKED_FIELDS_%d(fields); \\\n", i - 1);
>  
> +		printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", i);
>  		printf("})\n\n");
>  	}
>  
> 
> The problem is that, for some reason, it introduces this sparse warning:
> 
> ../lib/packing_test.c:436:9: warning: invalid access past the end of 'test_fields' (24 24)
> ../lib/packing_test.c:448:9: warning: invalid access past the end of 'test_fields' (24 24)
> 

I'll take a look and see if I can spot it.

> Nobody accesses past element 6 (ARRAY_SIZE) of test_fields[]. I ran the
> KUnit with kasan and I saw no warning. The strace warning comes from
> check_access() in flow.c, but I don't have any energy left today to go
> further into this.
> 
> I'm suspecting either a strace bug/false positive, or some sort of
> variable name aliasing issue which I haven't identified yet.

I had some issues with various attempts I made at the compiler checks
because different compilers would not always track constants.

Powered by blists - more mailing lists