[RFC net-next 0/2] introduce DECLARE_FLEX() macro

[RFC net-next 0/2] introduce DECLARE_FLEX() macro

[Przemek Kitszel]

Add DECLARE_FLEX() macro that lets us declare structures with flexible
array members on stack (patch 1).

Show how it could be used by ice example (note that post-RFC version will
cover whole driver, here is just one file) (patch 2).

Two open questions:
Any better macro name?
Especially given it's valid only for on stack allocations,
(ie. could not be placed into struct definitions).

More sugar?
It's tempting to introduce yet another macro that would avoid
repetition of flex array field name and count, eg to apply:
-struct_size(sw_buf, elem, 1)
+flex_sizeof(sw_buf)

With simple definition:
+#define flex_sizeof(var) \
+	sizeof(var##_buf)

Yet I'm unsure if usage it's not too magical then?

Przemek Kitszel (2):
  overflow: add DECLARE_FLEX() for on-stack allocs
  ice: make use of DECLARE_FLEX() in ice_switch.c

 drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
 include/linux/overflow.h                    | 14 ++++++
 2 files changed, 26 insertions(+), 41 deletions(-)


base-commit: 9d1505d88ad0f6970015a06a475b9d67b21f20fa
-- 
2.38.1

[RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Przemek Kitszel]

Add DECLARE_FLEX() macro for on-stack allocations of structs with
flexible array member.

Using underlying array for on-stack storage lets us to declare known
on compile-time structures without kzalloc().

Actual usage for ice driver is in next patch of the series.

Note that "struct" kw and "*" char is moved to the caller, to both:
have shorter macro name, and have more natural type specification
in the driver code (IOW not hiding an actual type of var).

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 include/linux/overflow.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f9b60313eaea..403b7ec120a2 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -309,4 +309,18 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
 #define struct_size_t(type, member, count)					\
 	struct_size((type *)NULL, member, count)
 
+/**
+ * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing
+ * flexible array.
+ * @type: Pointer to structure type, including "struct" keyword and "*" char.
+ * @name: Name for a (pointer) variable to create.
+ * @member: Name of the array member.
+ * @count: Number of elements in the array; must be compile-time const.
+ *
+ * Declare an instance of structure *@type with trailing flexible array.
+ */
+#define DECLARE_FLEX(type, name, member, count)					\
+	u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\
+	type name = (type)&name##_buf
+
 #endif /* __LINUX_OVERFLOW_H */
-- 
2.38.1

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Alexander Lobakin]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Tue, 1 Aug 2023 13:19:22 +0200

> Add DECLARE_FLEX() macro for on-stack allocations of structs with
> flexible array member.
> 
> Using underlying array for on-stack storage lets us to declare known
> on compile-time structures without kzalloc().
> 
> Actual usage for ice driver is in next patch of the series.
> 
> Note that "struct" kw and "*" char is moved to the caller, to both:
> have shorter macro name, and have more natural type specification
> in the driver code (IOW not hiding an actual type of var).
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  include/linux/overflow.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index f9b60313eaea..403b7ec120a2 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -309,4 +309,18 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>  #define struct_size_t(type, member, count)					\
>  	struct_size((type *)NULL, member, count)
>  
> +/**
> + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing
> + * flexible array.
> + * @type: Pointer to structure type, including "struct" keyword and "*" char.
> + * @name: Name for a (pointer) variable to create.
> + * @member: Name of the array member.
> + * @count: Number of elements in the array; must be compile-time const.
> + *
> + * Declare an instance of structure *@type with trailing flexible array.
> + */
> +#define DECLARE_FLEX(type, name, member, count)					\
> +	u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\

1. You can use struct_size_t() instead of open-coding it.
2. Maybe use alignof(type) instead of 8? Some structures have larger
   alignment requirements.

> +	type name = (type)&name##_buf

In general, I still think DECLARE_FLEX(struct foo) is better than
DECLARE_FLEX(struct foo *). Looking at container_of(), struct_size_t()
etc., they all take `type`, not `type *`, so even from the consistency
perspective your solution is not optimal to me.

> +
>  #endif /* __LINUX_OVERFLOW_H */

Thanks,
Olek

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Przemek Kitszel]

On 8/1/23 15:54, Alexander Lobakin wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Date: Tue, 1 Aug 2023 13:19:22 +0200
> 
>> Add DECLARE_FLEX() macro for on-stack allocations of structs with
>> flexible array member.
>>
>> Using underlying array for on-stack storage lets us to declare known
>> on compile-time structures without kzalloc().
>>
>> Actual usage for ice driver is in next patch of the series.
>>
>> Note that "struct" kw and "*" char is moved to the caller, to both:
>> have shorter macro name, and have more natural type specification
>> in the driver code (IOW not hiding an actual type of var).
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>>   include/linux/overflow.h | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>> index f9b60313eaea..403b7ec120a2 100644
>> --- a/include/linux/overflow.h
>> +++ b/include/linux/overflow.h
>> @@ -309,4 +309,18 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>>   #define struct_size_t(type, member, count)					\
>>   	struct_size((type *)NULL, member, count)
>>   
>> +/**
>> + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing
>> + * flexible array.
>> + * @type: Pointer to structure type, including "struct" keyword and "*" char.
>> + * @name: Name for a (pointer) variable to create.
>> + * @member: Name of the array member.
>> + * @count: Number of elements in the array; must be compile-time const.
>> + *
>> + * Declare an instance of structure *@type with trailing flexible array.
>> + */
>> +#define DECLARE_FLEX(type, name, member, count)					\
>> +	u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\
> 
> 1. You can use struct_size_t() instead of open-coding it.

with ptr param, not feasible, but otherwise, of course will do it (see 
below)

> 2. Maybe use alignof(type) instead of 8? Some structures have larger
>     alignment requirements.

Sure, thanks!

> 
>> +	type name = (type)&name##_buf
> 
> In general, I still think DECLARE_FLEX(struct foo) is better than
> DECLARE_FLEX(struct foo *).

I have started with that version, and that would prevent your question 
no. 1 :) So there is additional advantage to that.

> Looking at container_of(), struct_size_t()
> etc., they all take `type`, not `type *`, so even from the consistency
> perspective your solution is not optimal to me.

The two you have mentioned are "getter" macros. Random two from me, that 
actually declare something are:

#define DEVICE_ATTR_RW(_name) \
	struct device_attribute dev_attr_##_name = __ATTR_RW(_name)

#define DECLARE_BITMAP(name, bits) \
	unsigned long name[BITS_TO_LONGS(bits)]

Even if they don't take @type param, they declare variable of some 
non-pointer type.

Both variants have some logic that supports them, and some disadvantages:
ptr-arg: user declares sth as ptr, but it takes "a lot" of space
just-type-arg: user declares foo, but it's "*foo" actually, so "foo.bar" 
does not work.

I have no strong opinion here, so will just switch to pure-type param.

> Thanks,
> Olek

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Alexander Lobakin]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Tue, 1 Aug 2023 16:18:44 +0200

> On 8/1/23 15:54, Alexander Lobakin wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Date: Tue, 1 Aug 2023 13:19:22 +0200
>>
>>> Add DECLARE_FLEX() macro for on-stack allocations of structs with
>>> flexible array member.
>>>
>>> Using underlying array for on-stack storage lets us to declare known
>>> on compile-time structures without kzalloc().
>>>
>>> Actual usage for ice driver is in next patch of the series.
>>>
>>> Note that "struct" kw and "*" char is moved to the caller, to both:
>>> have shorter macro name, and have more natural type specification
>>> in the driver code (IOW not hiding an actual type of var).
>>>
>>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>> ---
>>>   include/linux/overflow.h | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>>> index f9b60313eaea..403b7ec120a2 100644
>>> --- a/include/linux/overflow.h
>>> +++ b/include/linux/overflow.h
>>> @@ -309,4 +309,18 @@ static inline size_t __must_check
>>> size_sub(size_t minuend, size_t subtrahend)
>>>   #define struct_size_t(type, member, count)                    \
>>>       struct_size((type *)NULL, member, count)
>>>   +/**
>>> + * DECLARE_FLEX() - Declare an on-stack instance of structure with
>>> trailing
>>> + * flexible array.
>>> + * @type: Pointer to structure type, including "struct" keyword and
>>> "*" char.
>>> + * @name: Name for a (pointer) variable to create.
>>> + * @member: Name of the array member.
>>> + * @count: Number of elements in the array; must be compile-time const.
>>> + *
>>> + * Declare an instance of structure *@type with trailing flexible
>>> array.
>>> + */
>>> +#define DECLARE_FLEX(type, name, member, count)                    \
>>> +    u8 name##_buf[struct_size((type)NULL, member, count)]
>>> __aligned(8) = {};\
>>
>> 1. You can use struct_size_t() instead of open-coding it.
> 
> with ptr param, not feasible, but otherwise, of course will do it (see

struct_size_t(typeof(*(type)NULL), member, count)

Jokin :D

> below)
> 
>> 2. Maybe use alignof(type) instead of 8? Some structures have larger
>>     alignment requirements.
> 
> Sure, thanks!
> 
>>
>>> +    type name = (type)&name##_buf
>>
>> In general, I still think DECLARE_FLEX(struct foo) is better than
>> DECLARE_FLEX(struct foo *).
> 
> I have started with that version, and that would prevent your question
> no. 1 :) So there is additional advantage to that.
> 
>> Looking at container_of(), struct_size_t()
>> etc., they all take `type`, not `type *`, so even from the consistency
>> perspective your solution is not optimal to me.
> 
> The two you have mentioned are "getter" macros. Random two from me, that
> actually declare something are:
> 
> #define DEVICE_ATTR_RW(_name) \
>     struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
> 
> #define DECLARE_BITMAP(name, bits) \
>     unsigned long name[BITS_TO_LONGS(bits)]
> 
> Even if they don't take @type param, they declare variable of some
> non-pointer type.
> 
> Both variants have some logic that supports them, and some disadvantages:
> ptr-arg: user declares sth as ptr, but it takes "a lot" of space
> just-type-arg: user declares foo, but it's "*foo" actually, so "foo.bar"
> does not work.

Same as DECLARE_BITMAP() actually: it always declares an array, so that
it's then __set_bit(FOO, bitmap), not __set_bit(FOO, &bitmap).

One more argument for "just-type": yes, the name you pass to the macro
is exported as a pointer, but you occupy the size of the type (plus tail
elements), not a pointer, on the stack.

> 
> I have no strong opinion here, so will just switch to pure-type param.
> 
>> Thanks,
>> Olek
> 

Thanks,
Olek

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Kees Cook]

On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote:
> Add DECLARE_FLEX() macro for on-stack allocations of structs with
> flexible array member.

I like this idea!

One terminology nit: I think this should be called "DEFINE_...", since
it's a specific instantiation. Other macros in the kernel seem to confuse
this a lot, though. Yay naming.

> Using underlying array for on-stack storage lets us to declare known
> on compile-time structures without kzalloc().

Hmpf, this appears to immediately trip over any (future) use of
__counted_by()[1] for these (since the counted-by member would be
initialized to zero), but I think I have a solution. (See below.)

> 
> Actual usage for ice driver is in next patch of the series.
> 
> Note that "struct" kw and "*" char is moved to the caller, to both:
> have shorter macro name, and have more natural type specification
> in the driver code (IOW not hiding an actual type of var).
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  include/linux/overflow.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index f9b60313eaea..403b7ec120a2 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -309,4 +309,18 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>  #define struct_size_t(type, member, count)					\
>  	struct_size((type *)NULL, member, count)
>  
> +/**
> + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing
> + * flexible array.
> + * @type: Pointer to structure type, including "struct" keyword and "*" char.
> + * @name: Name for a (pointer) variable to create.
> + * @member: Name of the array member.
> + * @count: Number of elements in the array; must be compile-time const.
> + *
> + * Declare an instance of structure *@type with trailing flexible array.
> + */
> +#define DECLARE_FLEX(type, name, member, count)					\
> +	u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\
> +	type name = (type)&name##_buf
> +
>  #endif /* __LINUX_OVERFLOW_H */

I was disappointed to discover that only global (static) initializers
would work for a flex array member. :(

i.e. this works:

struct foo {
    unsigned long flags;
    unsigned char count;
    int array[] __counted_by(count);
};

struct foo global = {
    .count = 1,
    .array = { 0 },
};

But I can't do that on the stack. :P So, yes, it seems like the u8 array
trick is needed.

It looks like Alexander already suggested this, and I agree, instead of
__aligned(8), please use "__aligned(_Alignof(type))".

As for "*" or not, I would tend to agree that always requiring "*" when
using the macro seems redundant.

Initially I was struggling to make __counted_by work, but it seems we can
use an initializer for that member, as long as we don't touch the flexible
array member in the initializer. So we just need to add the counted-by
member to the macro, and use a union to do the initialization. And if
we take the address of the union (and not the struct within it), the
compiler will see the correct object size with __builtin_object_size:

#define DEFINE_FLEX(type, name, flex, counter, count) \
    union { \
        u8   bytes[struct_size_t(type, flex, count)]; \
        type obj; \
    } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
    /* take address of whole union to get the correct __builtin_object_size */ \
    type *name = (type *)&name##_u

i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
works correctly here, but breaks (sees a zero-sized flex array member)
if this macro ends with:

    type *name = &name##_u.obj


-Kees

[1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb

-- 
Kees Cook

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Przemek Kitszel]

On 8/2/23 00:31, Kees Cook wrote:
> On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote:
>> Add DECLARE_FLEX() macro for on-stack allocations of structs with
>> flexible array member.
> 
> I like this idea!
> 
> One terminology nit: I think this should be called "DEFINE_...", since
> it's a specific instantiation. Other macros in the kernel seem to confuse
> this a lot, though. Yay naming.

Thanks, makes sense!

> 
>> Using underlying array for on-stack storage lets us to declare known
>> on compile-time structures without kzalloc().
> 
> Hmpf, this appears to immediately trip over any (future) use of
> __counted_by()[1] for these (since the counted-by member would be
> initialized to zero), but I think I have a solution. (See below.)
> 
>>
>> Actual usage for ice driver is in next patch of the series.
>>
>> Note that "struct" kw and "*" char is moved to the caller, to both:
>> have shorter macro name, and have more natural type specification
>> in the driver code (IOW not hiding an actual type of var).
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>>   include/linux/overflow.h | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>> index f9b60313eaea..403b7ec120a2 100644
>> --- a/include/linux/overflow.h
>> +++ b/include/linux/overflow.h
>> @@ -309,4 +309,18 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>>   #define struct_size_t(type, member, count)					\
>>   	struct_size((type *)NULL, member, count)
>>   
>> +/**
>> + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing
>> + * flexible array.
>> + * @type: Pointer to structure type, including "struct" keyword and "*" char.
>> + * @name: Name for a (pointer) variable to create.
>> + * @member: Name of the array member.
>> + * @count: Number of elements in the array; must be compile-time const.
>> + *
>> + * Declare an instance of structure *@type with trailing flexible array.
>> + */
>> +#define DECLARE_FLEX(type, name, member, count)					\
>> +	u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\
>> +	type name = (type)&name##_buf
>> +
>>   #endif /* __LINUX_OVERFLOW_H */
> 
> I was disappointed to discover that only global (static) initializers
> would work for a flex array member. :(
> 
> i.e. this works:
> 
> struct foo {
>      unsigned long flags;
>      unsigned char count;

So bad that in the ice driver (perhaps others too), we have cases that 
there is no counter or it has different meaning.
(potentially "complicated" meaning - ice' struct 
ice_aqc_alloc_free_res_elem has "__le16 num_elems", so could not be used 
verbatim, and it's not actual counter either :/ (I was fooled by such 
assumption here [2]). Perhaps recent series by Olek [3] is also good 
illustration of hard cases for __counted_by()

>      int array[] __counted_by(count);
> };
> 
> struct foo global = {
>      .count = 1,
>      .array = { 0 },
> };
> 
> But I can't do that on the stack. :P So, yes, it seems like the u8 array
> trick is needed.
> 
> It looks like Alexander already suggested this, and I agree, instead of
> __aligned(8), please use "__aligned(_Alignof(type))".
> 
> As for "*" or not, I would tend to agree that always requiring "*" when
> using the macro seems redundant.
> 
> Initially I was struggling to make __counted_by work, but it seems we can
> use an initializer for that member, as long as we don't touch the flexible
> array member in the initializer. So we just need to add the counted-by
> member to the macro, and use a union to do the initialization. And if
> we take the address of the union (and not the struct within it), the
> compiler will see the correct object size with __builtin_object_size:
> 
> #define DEFINE_FLEX(type, name, flex, counter, count) \
>      union { \
>          u8   bytes[struct_size_t(type, flex, count)]; \
>          type obj; \
>      } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
>      /* take address of whole union to get the correct __builtin_object_size */ \
>      type *name = (type *)&name##_u
> 
> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
> works correctly here, but breaks (sees a zero-sized flex array member)
> if this macro ends with:
> 
>      type *name = &name##_u.obj
> 
> 
> -Kees

I like the union usage here, it's a bit cleaner too.
For the counter param [for my, perhaps other] usages it does not fit 
well (as of now) :/.

I will post a v1 series to move this forward though :)

> 
> [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb
> 

[2] 
https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20230731150152.514984-1-przemyslaw.kitszel@intel.com/
in that [2], I have this:
-	cmd->num_entries = cpu_to_le16(num_entries);
+	cmd->num_entries = cpu_to_le16(1);
as "num_entries" is not a flex array counter :/

[3] 
https://lore.kernel.org/netdev/a8466f4b-f773-4d0a-f22b-34c83a7aa942@intel.com/T/

-Przemek

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Alexander Lobakin]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Fri, 4 Aug 2023 12:59:08 +0200

> On 8/2/23 00:31, Kees Cook wrote:
>> On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote:
>>> Add DECLARE_FLEX() macro for on-stack allocations of structs with
>>> flexible array member.
>>
>> I like this idea!
>>
>> One terminology nit: I think this should be called "DEFINE_...", since
>> it's a specific instantiation. Other macros in the kernel seem to confuse
>> this a lot, though. Yay naming.
> 
> Thanks, makes sense!
> 
>>
>>> Using underlying array for on-stack storage lets us to declare known
>>> on compile-time structures without kzalloc().
>>
>> Hmpf, this appears to immediately trip over any (future) use of
>> __counted_by()[1] for these (since the counted-by member would be
>> initialized to zero), but I think I have a solution. (See below.)
>>
>>>
>>> Actual usage for ice driver is in next patch of the series.
>>>
>>> Note that "struct" kw and "*" char is moved to the caller, to both:
>>> have shorter macro name, and have more natural type specification
>>> in the driver code (IOW not hiding an actual type of var).
>>>
>>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>> ---
>>>   include/linux/overflow.h | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>>> index f9b60313eaea..403b7ec120a2 100644
>>> --- a/include/linux/overflow.h
>>> +++ b/include/linux/overflow.h
>>> @@ -309,4 +309,18 @@ static inline size_t __must_check
>>> size_sub(size_t minuend, size_t subtrahend)
>>>   #define struct_size_t(type, member, count)                    \
>>>       struct_size((type *)NULL, member, count)
>>>   +/**
>>> + * DECLARE_FLEX() - Declare an on-stack instance of structure with
>>> trailing
>>> + * flexible array.
>>> + * @type: Pointer to structure type, including "struct" keyword and
>>> "*" char.
>>> + * @name: Name for a (pointer) variable to create.
>>> + * @member: Name of the array member.
>>> + * @count: Number of elements in the array; must be compile-time const.
>>> + *
>>> + * Declare an instance of structure *@type with trailing flexible
>>> array.
>>> + */
>>> +#define DECLARE_FLEX(type, name, member, count)                    \
>>> +    u8 name##_buf[struct_size((type)NULL, member, count)]
>>> __aligned(8) = {};\
>>> +    type name = (type)&name##_buf
>>> +
>>>   #endif /* __LINUX_OVERFLOW_H */
>>
>> I was disappointed to discover that only global (static) initializers
>> would work for a flex array member. :(
>>
>> i.e. this works:
>>
>> struct foo {
>>      unsigned long flags;
>>      unsigned char count;
> 
> So bad that in the ice driver (perhaps others too), we have cases that
> there is no counter or it has different meaning.
> (potentially "complicated" meaning - ice' struct
> ice_aqc_alloc_free_res_elem has "__le16 num_elems", so could not be used
> verbatim, and it's not actual counter either :/ (I was fooled by such

Speaking of __le16 (we already discussed it 1:1): it's not a rare case
to define Endianness-sensitive structures with a flex array at the end,
so for those with __{be,le}* we could be adding __counted_by() attribute
only when the host Endianness matches the structure's to have at least
some coverage. By "some" I mean actually a lot when it comes to LE
structures, which usually is the case :)

> assumption here [2]). Perhaps recent series by Olek [3] is also good
> illustration of hard cases for __counted_by()
> 
>>      int array[] __counted_by(count);
>> };

Thanks,
Olek

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Przemek Kitszel]

On 8/2/23 00:31, Kees Cook wrote:

[...]

> Initially I was struggling to make __counted_by work, but it seems we can
> use an initializer for that member, as long as we don't touch the flexible
> array member in the initializer. So we just need to add the counted-by
> member to the macro, and use a union to do the initialization. And if
> we take the address of the union (and not the struct within it), the
> compiler will see the correct object size with __builtin_object_size:
> 
> #define DEFINE_FLEX(type, name, flex, counter, count) \
>      union { \
>          u8   bytes[struct_size_t(type, flex, count)]; \
>          type obj; \
>      } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
>      /* take address of whole union to get the correct __builtin_object_size */ \
>      type *name = (type *)&name##_u
> 
> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
> works correctly here, but breaks (sees a zero-sized flex array member)
> if this macro ends with:
> 
>      type *name = &name##_u.obj

__builtin_object_size(name, 0) works fine for both versions (with and 
without .obj at the end)

however it does not work for builds without -O2 switch, so 
struct_size_t() is rather a way to go :/

> 
> 
> -Kees
> 
> [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb
> 

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Alexander Lobakin]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Fri, 4 Aug 2023 15:47:48 +0200

> On 8/2/23 00:31, Kees Cook wrote:
> 
> [...]
> 
>> Initially I was struggling to make __counted_by work, but it seems we can
>> use an initializer for that member, as long as we don't touch the
>> flexible
>> array member in the initializer. So we just need to add the counted-by
>> member to the macro, and use a union to do the initialization. And if
>> we take the address of the union (and not the struct within it), the
>> compiler will see the correct object size with __builtin_object_size:
>>
>> #define DEFINE_FLEX(type, name, flex, counter, count) \
>>      union { \
>>          u8   bytes[struct_size_t(type, flex, count)]; \
>>          type obj; \
>>      } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
>>      /* take address of whole union to get the correct
>> __builtin_object_size */ \
>>      type *name = (type *)&name##_u
>>
>> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
>> works correctly here, but breaks (sees a zero-sized flex array member)
>> if this macro ends with:
>>
>>      type *name = &name##_u.obj
> 
> __builtin_object_size(name, 0) works fine for both versions (with and
> without .obj at the end)
> 
> however it does not work for builds without -O2 switch, so
> struct_size_t() is rather a way to go :/

You only need to care about -O2 and -Os, since only those 2 are
officially supported by Kbuild. Did you mean it doesn't work on -Os as well?

> 
>>
>>
>> -Kees
>>
>> [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb
>>
> 

Thanks,
Olek

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Przemek Kitszel]

On 8/4/23 17:44, Alexander Lobakin wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Date: Fri, 4 Aug 2023 15:47:48 +0200
> 
>> On 8/2/23 00:31, Kees Cook wrote:
>>
>> [...]
>>
>>> Initially I was struggling to make __counted_by work, but it seems we can
>>> use an initializer for that member, as long as we don't touch the
>>> flexible
>>> array member in the initializer. So we just need to add the counted-by
>>> member to the macro, and use a union to do the initialization. And if
>>> we take the address of the union (and not the struct within it), the
>>> compiler will see the correct object size with __builtin_object_size:
>>>
>>> #define DEFINE_FLEX(type, name, flex, counter, count) \
>>>       union { \
>>>           u8   bytes[struct_size_t(type, flex, count)]; \
>>>           type obj; \
>>>       } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
>>>       /* take address of whole union to get the correct
>>> __builtin_object_size */ \
>>>       type *name = (type *)&name##_u
>>>
>>> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
>>> works correctly here, but breaks (sees a zero-sized flex array member)
>>> if this macro ends with:
>>>
>>>       type *name = &name##_u.obj
>>
>> __builtin_object_size(name, 0) works fine for both versions (with and
>> without .obj at the end)
>>
>> however it does not work for builds without -O2 switch, so
>> struct_size_t() is rather a way to go :/
> 
> You only need to care about -O2 and -Os, since only those 2 are
> officially supported by Kbuild. Did you mean it doesn't work on -Os as well?

Both -Os and -O2 are fine here.

One thing is that perhaps a "user friendly" define for 
"__builtin_object_size(name, 1)" would avoid any potential for 
misleading "1" with any "counter" variable, will see.

> 
>>
>>>
>>>
>>> -Kees
>>>
>>> [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb
>>>
>>
> 
> Thanks,
> Olek

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Kees Cook]

On August 7, 2023 5:42:31 AM PDT, Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote:
>On 8/4/23 17:44, Alexander Lobakin wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Date: Fri, 4 Aug 2023 15:47:48 +0200
>> 
>>> On 8/2/23 00:31, Kees Cook wrote:
>>> 
>>> [...]
>>> 
>>>> Initially I was struggling to make __counted_by work, but it seems we can
>>>> use an initializer for that member, as long as we don't touch the
>>>> flexible
>>>> array member in the initializer. So we just need to add the counted-by
>>>> member to the macro, and use a union to do the initialization. And if
>>>> we take the address of the union (and not the struct within it), the
>>>> compiler will see the correct object size with __builtin_object_size:
>>>> 
>>>> #define DEFINE_FLEX(type, name, flex, counter, count) \
>>>>       union { \
>>>>           u8   bytes[struct_size_t(type, flex, count)]; \
>>>>           type obj; \
>>>>       } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
>>>>       /* take address of whole union to get the correct
>>>> __builtin_object_size */ \
>>>>       type *name = (type *)&name##_u
>>>> 
>>>> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
>>>> works correctly here, but breaks (sees a zero-sized flex array member)
>>>> if this macro ends with:
>>>> 
>>>>       type *name = &name##_u.obj
>>> 
>>> __builtin_object_size(name, 0) works fine for both versions (with and
>>> without .obj at the end)
>>> 
>>> however it does not work for builds without -O2 switch, so
>>> struct_size_t() is rather a way to go :/
>> 
>> You only need to care about -O2 and -Os, since only those 2 are
>> officially supported by Kbuild. Did you mean it doesn't work on -Os as well?
>
>Both -Os and -O2 are fine here.
>
>One thing is that perhaps a "user friendly" define for "__builtin_object_size(name, 1)" would avoid any potential for misleading "1" with any "counter" variable, will see.

I meant that FORTIFY_SOURCE will work correctly in the example I gave. (__builtin_object_size() is used there.) Also note that "max size" (mode 0) is unchanged in either case, but mode 1 needs to see the outer object to get the min size correct.

-Kees


-- 
Kees Cook

Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

[Kees Cook]

On Fri, Aug 04, 2023 at 03:47:48PM +0200, Przemek Kitszel wrote:
> On 8/2/23 00:31, Kees Cook wrote:
> 
> [...]
> 
> > Initially I was struggling to make __counted_by work, but it seems we can
> > use an initializer for that member, as long as we don't touch the flexible
> > array member in the initializer. So we just need to add the counted-by
> > member to the macro, and use a union to do the initialization. And if
> > we take the address of the union (and not the struct within it), the
> > compiler will see the correct object size with __builtin_object_size:
> > 
> > #define DEFINE_FLEX(type, name, flex, counter, count) \
> >      union { \
> >          u8   bytes[struct_size_t(type, flex, count)]; \
> >          type obj; \
> >      } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
> >      /* take address of whole union to get the correct __builtin_object_size */ \
> >      type *name = (type *)&name##_u
> > 
> > i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
> > works correctly here, but breaks (sees a zero-sized flex array member)
> > if this macro ends with:
> > 
> >      type *name = &name##_u.obj
> 
> __builtin_object_size(name, 0) works fine for both versions (with and
> without .obj at the end)

Mode 0 will be unchanged, but mode 1 is what most of FORTIFY uses for
keep the scope narrow.

> however it does not work for builds without -O2 switch, so struct_size_t()
> is rather a way to go :/

FORTIFY depends on -O2, so no worries. :)

-- 
Kees Cook

[RFC net-next 2/2] ice: make use of DECLARE_FLEX() in ice_switch.c

[Przemek Kitszel]

Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
 1 file changed, 12 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index a7afb612fe32..41679cb6b548 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -1812,15 +1812,11 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
 			   enum ice_sw_lkup_type lkup_type,
 			   enum ice_adminq_opc opc)
 {
-	struct ice_aqc_alloc_free_res_elem *sw_buf;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
+	u16 buf_len = struct_size(sw_buf, elem, 1);
 	struct ice_aqc_res_elem *vsi_ele;
-	u16 buf_len;
 	int status;
 
-	buf_len = struct_size(sw_buf, elem, 1);
-	sw_buf = devm_kzalloc(ice_hw_to_dev(hw), buf_len, GFP_KERNEL);
-	if (!sw_buf)
-		return -ENOMEM;
 	sw_buf->num_elems = cpu_to_le16(1);
 
 	if (lkup_type == ICE_SW_LKUP_MAC ||
@@ -1840,8 +1836,7 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
 			sw_buf->res_type =
 				cpu_to_le16(ICE_AQC_RES_TYPE_VSI_LIST_PRUNE);
 	} else {
-		status = -EINVAL;
-		goto ice_aq_alloc_free_vsi_list_exit;
+		return -EINVAL;
 	}
 
 	if (opc == ice_aqc_opc_free_res)
@@ -1849,16 +1844,14 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
 
 	status = ice_aq_alloc_free_res(hw, 1, sw_buf, buf_len, opc, NULL);
 	if (status)
-		goto ice_aq_alloc_free_vsi_list_exit;
+		return status;
 
 	if (opc == ice_aqc_opc_alloc_res) {
 		vsi_ele = &sw_buf->elem[0];
 		*vsi_list_id = le16_to_cpu(vsi_ele->e.sw_resp);
 	}
 
-ice_aq_alloc_free_vsi_list_exit:
-	devm_kfree(ice_hw_to_dev(hw), sw_buf);
-	return status;
+	return 0;
 }
 
 /**
@@ -2088,15 +2081,10 @@ ice_aq_get_recipe_to_profile(struct ice_hw *hw, u32 profile_id, u8 *r_bitmap,
  */
 int ice_alloc_recipe(struct ice_hw *hw, u16 *rid)
 {
-	struct ice_aqc_alloc_free_res_elem *sw_buf;
-	u16 buf_len;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
+	u16 buf_len = struct_size(sw_buf, elem, 1);
 	int status;
 
-	buf_len = struct_size(sw_buf, elem, 1);
-	sw_buf = kzalloc(buf_len, GFP_KERNEL);
-	if (!sw_buf)
-		return -ENOMEM;
-
 	sw_buf->num_elems = cpu_to_le16(1);
 	sw_buf->res_type = cpu_to_le16((ICE_AQC_RES_TYPE_RECIPE <<
 					ICE_AQC_RES_TYPE_S) |
@@ -2105,7 +2093,6 @@ int ice_alloc_recipe(struct ice_hw *hw, u16 *rid)
 				       ice_aqc_opc_alloc_res, NULL);
 	if (!status)
 		*rid = le16_to_cpu(sw_buf->elem[0].e.sw_resp);
-	kfree(sw_buf);
 
 	return status;
 }
@@ -4482,16 +4469,10 @@ int
 ice_alloc_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 		   u16 *counter_id)
 {
-	struct ice_aqc_alloc_free_res_elem *buf;
-	u16 buf_len;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, buf, elem, 1);
+	u16 buf_len = struct_size(buf, elem, 1);
 	int status;
 
-	/* Allocate resource */
-	buf_len = struct_size(buf, elem, 1);
-	buf = kzalloc(buf_len, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	buf->num_elems = cpu_to_le16(num_items);
 	buf->res_type = cpu_to_le16(((type << ICE_AQC_RES_TYPE_S) &
 				      ICE_AQC_RES_TYPE_M) | alloc_shared);
@@ -4499,12 +4480,9 @@ ice_alloc_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 	status = ice_aq_alloc_free_res(hw, 1, buf, buf_len,
 				       ice_aqc_opc_alloc_res, NULL);
 	if (status)
-		goto exit;
+		return status;
 
 	*counter_id = le16_to_cpu(buf->elem[0].e.sw_resp);
-
-exit:
-	kfree(buf);
 	return status;
 }
 
@@ -4520,16 +4498,10 @@ int
 ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 		  u16 counter_id)
 {
-	struct ice_aqc_alloc_free_res_elem *buf;
-	u16 buf_len;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, buf, elem, 1);
+	u16 buf_len = struct_size(buf, elem, 1);
 	int status;
 
-	/* Free resource */
-	buf_len = struct_size(buf, elem, 1);
-	buf = kzalloc(buf_len, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	buf->num_elems = cpu_to_le16(num_items);
 	buf->res_type = cpu_to_le16(((type << ICE_AQC_RES_TYPE_S) &
 				      ICE_AQC_RES_TYPE_M) | alloc_shared);
@@ -4540,7 +4512,6 @@ ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 	if (status)
 		ice_debug(hw, ICE_DBG_SW, "counter resource could not be freed\n");
 
-	kfree(buf);
 	return status;
 }
 
-- 
2.38.1

Re: [RFC net-next 2/2] ice: make use of DECLARE_FLEX() in ice_switch.c

[Alexander Lobakin]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Tue, 1 Aug 2023 13:19:23 +0200

> Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
>  1 file changed, 12 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> index a7afb612fe32..41679cb6b548 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -1812,15 +1812,11 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>  			   enum ice_sw_lkup_type lkup_type,
>  			   enum ice_adminq_opc opc)
>  {
> -	struct ice_aqc_alloc_free_res_elem *sw_buf;
> +	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
> +	u16 buf_len = struct_size(sw_buf, elem, 1);
>  	struct ice_aqc_res_elem *vsi_ele;
> -	u16 buf_len;
>  	int status;
>  
> -	buf_len = struct_size(sw_buf, elem, 1);
> -	sw_buf = devm_kzalloc(ice_hw_to_dev(hw), buf_len, GFP_KERNEL);
> -	if (!sw_buf)
> -		return -ENOMEM;
>  	sw_buf->num_elems = cpu_to_le16(1);
>  
>  	if (lkup_type == ICE_SW_LKUP_MAC ||
> @@ -1840,8 +1836,7 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>  			sw_buf->res_type =
>  				cpu_to_le16(ICE_AQC_RES_TYPE_VSI_LIST_PRUNE);
>  	} else {
> -		status = -EINVAL;
> -		goto ice_aq_alloc_free_vsi_list_exit;
> +		return -EINVAL;
>  	}
>  
>  	if (opc == ice_aqc_opc_free_res)

bloat-o-meter results would be nice to have in the commitmsg.

[...]

Thanks,
Olek

Re: [RFC net-next 2/2] ice: make use of DECLARE_FLEX() in ice_switch.c

[Przemek Kitszel]

On 8/1/23 15:48, Alexander Lobakin wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Date: Tue, 1 Aug 2023 13:19:23 +0200
> 
>> Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
>>   1 file changed, 12 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
>> index a7afb612fe32..41679cb6b548 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
>> @@ -1812,15 +1812,11 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>>   			   enum ice_sw_lkup_type lkup_type,
>>   			   enum ice_adminq_opc opc)
>>   {
>> -	struct ice_aqc_alloc_free_res_elem *sw_buf;
>> +	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
>> +	u16 buf_len = struct_size(sw_buf, elem, 1);
>>   	struct ice_aqc_res_elem *vsi_ele;
>> -	u16 buf_len;
>>   	int status;
>>   
>> -	buf_len = struct_size(sw_buf, elem, 1);
>> -	sw_buf = devm_kzalloc(ice_hw_to_dev(hw), buf_len, GFP_KERNEL);
>> -	if (!sw_buf)
>> -		return -ENOMEM;
>>   	sw_buf->num_elems = cpu_to_le16(1);
>>   
>>   	if (lkup_type == ICE_SW_LKUP_MAC ||
>> @@ -1840,8 +1836,7 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>>   			sw_buf->res_type =
>>   				cpu_to_le16(ICE_AQC_RES_TYPE_VSI_LIST_PRUNE);
>>   	} else {
>> -		status = -EINVAL;
>> -		goto ice_aq_alloc_free_vsi_list_exit;
>> +		return -EINVAL;
>>   	}
>>   
>>   	if (opc == ice_aqc_opc_free_res)
> 
> bloat-o-meter results would be nice to have in the commitmsg.

I will do next time, perhaps you could tell me if I get the results 
right here:

./scripts/bloat-o-meter ice_switch.o{ld,}
add/remove: 2/2 grow/shrink: 3/5 up/down: 560/-483 (77)
Function                                     old     new   delta
ice_create_vsi_list_rule                       -     241    +241
ice_remove_vsi_list_rule                     139     270    +131
ice_add_adv_rule                            6047    6139     +92
ice_add_sw_recipe                           2892    2972     +80
__pfx_ice_create_vsi_list_rule                 -      16     +16
ice_alloc_recipe                             124     113     -11
__pfx_ice_aq_alloc_free_vsi_list              16       -     -16
ice_free_res_cntr                            185     155     -30
ice_alloc_res_cntr                           154     124     -30
ice_add_update_vsi_list                     1037     994     -43
ice_add_vlan_internal                       1027     953     -74
ice_aq_alloc_free_vsi_list                   279       -    -279
Total: Before=42183, After=42260, chg +0.18%

My guess here is that compiler did different decisions about what to 
inline where, what is biggest difference.
And biggest gain here is avoidance of heap allocs, perhaps that enables 
gcc to shuffle things a bit too.
Another guess is that b-o-m ignores heap bloat, so slight growth is 
expected.

Values reported for ice.ko are the same, with bigger base to compute the 
percent off.

 > [...]
 >
 > Thanks,
 > Olek

Thank you too, also for our initial talk about on the topic.

Re: [RFC net-next 2/2] ice: make use of DECLARE_FLEX() in ice_switch.c

[Alexander Lobakin]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Tue, 1 Aug 2023 16:36:57 +0200

> On 8/1/23 15:48, Alexander Lobakin wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Date: Tue, 1 Aug 2023 13:19:23 +0200

[...]

>>> -        status = -EINVAL;
>>> -        goto ice_aq_alloc_free_vsi_list_exit;
>>> +        return -EINVAL;
>>>       }
>>>         if (opc == ice_aqc_opc_free_res)
>>
>> bloat-o-meter results would be nice to have in the commitmsg.
> 
> I will do next time, perhaps you could tell me if I get the results
> right here:
> 
> ./scripts/bloat-o-meter ice_switch.o{ld,}
> add/remove: 2/2 grow/shrink: 3/5 up/down: 560/-483 (77)
> Function                                     old     new   delta
> ice_create_vsi_list_rule                       -     241    +241
> ice_remove_vsi_list_rule                     139     270    +131
> ice_add_adv_rule                            6047    6139     +92
> ice_add_sw_recipe                           2892    2972     +80
> __pfx_ice_create_vsi_list_rule                 -      16     +16
> ice_alloc_recipe                             124     113     -11
> __pfx_ice_aq_alloc_free_vsi_list              16       -     -16
> ice_free_res_cntr                            185     155     -30
> ice_alloc_res_cntr                           154     124     -30
> ice_add_update_vsi_list                     1037     994     -43
> ice_add_vlan_internal                       1027     953     -74
> ice_aq_alloc_free_vsi_list                   279       -    -279
> Total: Before=42183, After=42260, chg +0.18%
> 
> My guess here is that compiler did different decisions about what to
> inline where, what is biggest difference.

77 bytes is very good result, because see below.

> And biggest gain here is avoidance of heap allocs, perhaps that enables
> gcc to shuffle things a bit too.

Exactly, having the stack grown only by 77 bytes after avoiding -- how
many? -- a lot of heap allocations sounds great.

> Another guess is that b-o-m ignores heap bloat, so slight growth is
> expected.

BOM can't calculate any heap usage, it simply compares symbol sizes in
object files.

(BTW, passing /dev/null as the first "object file" is legit, in case you
 just want to see sorted symbol sizes in your module or vmlinux)

> 
> Values reported for ice.ko are the same, with bigger base to compute the
> percent off.
> 
>> [...]
>>
>> Thanks,
>> Olek
> 
> Thank you too, also for our initial talk about on the topic.

No problem, I really feel like this macro would be very useful.

Thanks,
Olek

Re: [RFC net-next 2/2] ice: make use of DECLARE_FLEX() in ice_switch.c

[Kees Cook]

On Tue, Aug 01, 2023 at 01:19:23PM +0200, Przemek Kitszel wrote:
> Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
>  1 file changed, 12 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> index a7afb612fe32..41679cb6b548 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -1812,15 +1812,11 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>  			   enum ice_sw_lkup_type lkup_type,
>  			   enum ice_adminq_opc opc)
>  {
> -	struct ice_aqc_alloc_free_res_elem *sw_buf;
> +	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
> +	u16 buf_len = struct_size(sw_buf, elem, 1);

With the macro I suggested, I think this line can become:

	u16 buf_len = __builtin_object_size(sw_buf, 1);

but either is fine. (N.B. the "1" here is a bitfield, not the "1" size
above).

-Kees

-- 
Kees Cook