Re: [PATCH net 1/4] virtchnl: Fix layout of RSS structures

Hi Samudrala,

On Fri, Mar 26, 2021 at 11:45 PM Samudrala, Sridhar
[off-list ref] wrote:

On 3/26/2021 1:06 AM, Geert Uytterhoeven wrote:

quoted

On Thu, Mar 25, 2021 at 11:29 PM Tony Nguyen [off-list ref] wrote:
From: Norbert Ciosek <redacted>

Remove padding from RSS structures. Previous layout
could lead to unwanted compiler optimizations
in loops when iterating over key and lut arrays.

From an earlier private conversation with Mateusz, I understand the real
explanation is that key[] and lut[] must be at the end of the
structures, because they are used as flexible array members?

Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
Signed-off-by: Norbert Ciosek <redacted>
Tested-by: Konrad Jankowski <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h

@@ -476,7 +476,6 @@ struct virtchnl_rss_key {
        u16 vsi_id;
        u16 key_len;
        u8 key[1];         /* RSS hash key, packed bytes */
-       u8 pad[1];
 };

 VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);

@@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
        u16 vsi_id;
        u16 lut_entries;
        u8 lut[1];        /* RSS lookup table */
-       u8 pad[1];
 };

If you use a flexible array member, it should be declared without a size,
i.e.

    u8 key[];

Everything else is (trying to) fool the compiler, and leading to undefined
behavior, and people (re)adding explicit padding.

This header file is shared across other OSes that use C++ that doesn't support
flexible arrays. So the structures in this file use an array of size 1 as a last
element to enable variable sized arrays.

I don't think it is accepted practice to have non-Linux-isms in
include/*linux*/avf/virtchnl.h header files.  Moreover, using a size
of 1 is counter-intuitive for people used to Linux kernel development,
and may lead to off-by-one errors in calculation of sizes.

If you insist on ignoring the above, this definitely deserves a
comment next to the member's declaration.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On 3/27/2021 2:53 AM, Geert Uytterhoeven wrote:

Hi Samudrala,

On Fri, Mar 26, 2021 at 11:45 PM Samudrala, Sridhar
[off-list ref] wrote:

quoted

On 3/26/2021 1:06 AM, Geert Uytterhoeven wrote:

quoted

On Thu, Mar 25, 2021 at 11:29 PM Tony Nguyen [off-list ref] wrote:
From: Norbert Ciosek <redacted>

Remove padding from RSS structures. Previous layout
could lead to unwanted compiler optimizations
in loops when iterating over key and lut arrays.

 From an earlier private conversation with Mateusz, I understand the real
explanation is that key[] and lut[] must be at the end of the
structures, because they are used as flexible array members?

Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
Signed-off-by: Norbert Ciosek <redacted>
Tested-by: Konrad Jankowski <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h

@@ -476,7 +476,6 @@ struct virtchnl_rss_key {
         u16 vsi_id;
         u16 key_len;
         u8 key[1];         /* RSS hash key, packed bytes */
-       u8 pad[1];
  };

  VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);

@@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
         u16 vsi_id;
         u16 lut_entries;
         u8 lut[1];        /* RSS lookup table */
-       u8 pad[1];
  };

If you use a flexible array member, it should be declared without a size,
i.e.

     u8 key[];

Everything else is (trying to) fool the compiler, and leading to undefined
behavior, and people (re)adding explicit padding.

This header file is shared across other OSes that use C++ that doesn't support
flexible arrays. So the structures in this file use an array of size 1 as a last
element to enable variable sized arrays.

I don't think it is accepted practice to have non-Linux-isms in
include/*linux*/avf/virtchnl.h header files.  Moreover, using a size
of 1 is counter-intuitive for people used to Linux kernel development,
and may lead to off-by-one errors in calculation of sizes.

If you insist on ignoring the above, this definitely deserves a
comment next to the member's declaration.

Sure. We can add a comment indicating that these fields are used 
variable sized arrays.

Thanks
Sridhar