Libguestfs - May 2023 - [libnbd PATCH] internal: Tweak use of attribute packed in union sbuf

While analyzing 'union sbuf' in preparation to add more members to the
union, I noticed several things related to __attribute__((packed))
that can be improved.  It helps to note that that the bulk of the
members of 'union sbuf' are already marked packed structures from
nbd-protocol.h, where we don't need to repeat that annotation in
internal.h but where it does factor into sbuf alignment.

First, rather than open-coding the attribute name in internal.h (in
some places with inconsistent whitespace), we can reuse
NBD_ATTRIBUTE_PACKED already present from nbd-protocol.h.

Second, note that two of the union members are themselves substructs
with two parts: both sbuf.or and sbuf.sr need distinct storage for a
header (a packed structure) and a payload (a union of various items;
currently each member of both of those unions are already packed),
where the choice of union branch and overall size of the payload to be
read (if any) is determined by information in the header.  Later
states then refer to information from both the header and payload, so
we need to keep the sub-structs (rather than hoisting the two parts of
the sub-struct into being directly-overlapping top-level members of
union sbuf proper).  But because we don't know the payload size until
after the header is read, the state machine uses separate recv() calls
into the two parts of sbuf.or and sbuf.sr; and there is no specific
reason that the two reads need to occur into adjacent memory.  Thus,
these two packed annotations buy us nothing at this layer and can be
safely elided.

Finally, there are benefits to naturally aligning uint64_t members,
whether or not hardware supports unaligned access.  Even though we are
using attribute packed to match wire format (and some NBD messages do
not have any natural alignment), the judicious use of a non-packed
uint64_t member to various unions gives the compiler permission to
insert padding that is otherwise not possible when a packed struct
occurs immediately before a union containing only packed members.  In
particular, with structured replies, it is worth ensuring that
sbuf.sr.payload.offset_data.offset falls on a 64-bit alignment.

Note that sbuf.or does not need this latter treatment (currently,
(sbuf.or.payload.export.exportsize is the only uint64_t in that
particular payload union, but does not occur at a natural alignment to
begin with); but it is also worth remembering that option negotiation
is not in the hot path the way sbuf.sr is.

Actually testing the alignment change from adding align_ members is
harder to do, especially without C11's <stdalign.h>, in part because
on 32-bit platforms where uint64_t is only 4-byte aligned (without gcc
-malign-double), there is no change to layout.  But in my
investigation under gdb on x86_64, inserting the otherwise-unused
align_* members changed both offsetof(struct nbd_handle, sbuf) % 8 and
(offsetof(struct nbd_handle, sbuf.sr.payload) - offsetof(struct
nbd_handle, sbuf)) % 8 from 4 to 0, which is what I wanted.

Not touched here: gcc's -Wpacked says several (but not all) of our
structures in that file are already naturally aligned, where adding a
packed notation can actually cause slower code when embedding that
type in a larger structure (exactly what we're doing when combining
those structs into sbuf), when the compiler has to prepare for
unaligned access even if the struct would otherwise be aligned.

Signed-off-by: Eric Blake <eblake at redhat.com>
---

Fallout from review of the 64-bit v3 2/22 patch.

 lib/internal.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index b155681d..a8c49e16 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -212,6 +212,7 @@ struct nbd_handle {
    * and commands.
    */
   union {
+    uint64_t align_; /* Start sbuf on an 8-byte alignment where useful */
     struct nbd_old_handshake old_handshake;
     struct nbd_new_handshake new_handshake;
     struct nbd_new_option option;
@@ -221,34 +222,35 @@ struct nbd_handle {
         struct {
           struct nbd_fixed_new_option_reply_server server;
           char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */
-        } __attribute__ ((packed)) server;
+        } NBD_ATTRIBUTE_PACKED server;
         struct nbd_fixed_new_option_reply_info_export export;
         struct nbd_fixed_new_option_reply_info_block_size block_size;
         struct {
           struct nbd_fixed_new_option_reply_info_name_or_desc info;
           char str[NBD_MAX_STRING];
-        } __attribute__ ((packed)) name_desc;
+        } NBD_ATTRIBUTE_PACKED name_desc;
         struct {
           struct nbd_fixed_new_option_reply_meta_context context;
           char str[NBD_MAX_STRING];
-        }  __attribute__ ((packed)) context;
+        } NBD_ATTRIBUTE_PACKED context;
         char err_msg[NBD_MAX_STRING];
       } payload;
-    }  __attribute__ ((packed)) or;
+    } or;
     struct nbd_export_name_option_reply export_name_reply;
     struct nbd_simple_reply simple_reply;
     struct {
       struct nbd_structured_reply structured_reply;
       union {
+        uint64_t align_; /* Start sr.payload on an 8-byte alignment */
         struct nbd_structured_reply_offset_data offset_data;
         struct nbd_structured_reply_offset_hole offset_hole;
         struct {
           struct nbd_structured_reply_error error;
           char msg[NBD_MAX_STRING]; /* Common to all error types */
           uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */
-        } __attribute__ ((packed)) error;
+        } NBD_ATTRIBUTE_PACKED error;
       } payload;
-    }  __attribute__ ((packed)) sr;
+    } sr;
     uint16_t gflags;
     uint32_t cflags;
     uint32_t len;
-- 
2.40.1

On Tue, May 30, 2023 at 09:10:13PM -0500, Eric Blake
wrote:
> While analyzing 'union sbuf' in preparation to add more members to
the
> union, I noticed several things related to __attribute__((packed))
> that can be improved.  It helps to note that that the bulk of the
> members of 'union sbuf' are already marked packed structures from
> nbd-protocol.h, where we don't need to repeat that annotation in
> internal.h but where it does factor into sbuf alignment.
> 
> First, rather than open-coding the attribute name in internal.h (in
> some places with inconsistent whitespace), we can reuse
> NBD_ATTRIBUTE_PACKED already present from nbd-protocol.h.
> 
> Second, note that two of the union members are themselves substructs
> with two parts: both sbuf.or and sbuf.sr need distinct storage for a
> header (a packed structure) and a payload (a union of various items;
> currently each member of both of those unions are already packed),
> where the choice of union branch and overall size of the payload to be
> read (if any) is determined by information in the header.  Later
> states then refer to information from both the header and payload, so
> we need to keep the sub-structs (rather than hoisting the two parts of
> the sub-struct into being directly-overlapping top-level members of
> union sbuf proper).  But because we don't know the payload size until
> after the header is read, the state machine uses separate recv() calls
> into the two parts of sbuf.or and sbuf.sr; and there is no specific
> reason that the two reads need to occur into adjacent memory.  Thus,
> these two packed annotations buy us nothing at this layer and can be
> safely elided.
> 
> Finally, there are benefits to naturally aligning uint64_t members,
> whether or not hardware supports unaligned access.  Even though we are
> using attribute packed to match wire format (and some NBD messages do
> not have any natural alignment), the judicious use of a non-packed
> uint64_t member to various unions gives the compiler permission to
> insert padding that is otherwise not possible when a packed struct
> occurs immediately before a union containing only packed members.  In
> particular, with structured replies, it is worth ensuring that
> sbuf.sr.payload.offset_data.offset falls on a 64-bit alignment.
> 
> Note that sbuf.or does not need this latter treatment (currently,
> (sbuf.or.payload.export.exportsize is the only uint64_t in that
> particular payload union, but does not occur at a natural alignment to
> begin with); but it is also worth remembering that option negotiation
> is not in the hot path the way sbuf.sr is.
> 
> Actually testing the alignment change from adding align_ members is
> harder to do, especially without C11's <stdalign.h>, in part
because
> on 32-bit platforms where uint64_t is only 4-byte aligned (without gcc
> -malign-double), there is no change to layout.  But in my
> investigation under gdb on x86_64, inserting the otherwise-unused
> align_* members changed both offsetof(struct nbd_handle, sbuf) % 8 and
> (offsetof(struct nbd_handle, sbuf.sr.payload) - offsetof(struct
> nbd_handle, sbuf)) % 8 from 4 to 0, which is what I wanted.
> 
> Not touched here: gcc's -Wpacked says several (but not all) of our
> structures in that file are already naturally aligned, where adding a
> packed notation can actually cause slower code when embedding that
> type in a larger structure (exactly what we're doing when combining
> those structs into sbuf), when the compiler has to prepare for
> unaligned access even if the struct would otherwise be aligned.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> Fallout from review of the 64-bit v3 2/22 patch.
> 
>  lib/internal.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index b155681d..a8c49e16 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -212,6 +212,7 @@ struct nbd_handle {
>     * and commands.
>     */
>    union {
> +    uint64_t align_; /* Start sbuf on an 8-byte alignment where useful */
>      struct nbd_old_handshake old_handshake;
>      struct nbd_new_handshake new_handshake;
>      struct nbd_new_option option;
> @@ -221,34 +222,35 @@ struct nbd_handle {
>          struct {
>            struct nbd_fixed_new_option_reply_server server;
>            char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */
> -        } __attribute__ ((packed)) server;
> +        } NBD_ATTRIBUTE_PACKED server;
>          struct nbd_fixed_new_option_reply_info_export export;
>          struct nbd_fixed_new_option_reply_info_block_size block_size;
>          struct {
>            struct nbd_fixed_new_option_reply_info_name_or_desc info;
>            char str[NBD_MAX_STRING];
> -        } __attribute__ ((packed)) name_desc;
> +        } NBD_ATTRIBUTE_PACKED name_desc;
>          struct {
>            struct nbd_fixed_new_option_reply_meta_context context;
>            char str[NBD_MAX_STRING];
> -        }  __attribute__ ((packed)) context;
> +        } NBD_ATTRIBUTE_PACKED context;
>          char err_msg[NBD_MAX_STRING];
>        } payload;
> -    }  __attribute__ ((packed)) or;
> +    } or;
>      struct nbd_export_name_option_reply export_name_reply;
>      struct nbd_simple_reply simple_reply;
>      struct {
>        struct nbd_structured_reply structured_reply;
>        union {
> +        uint64_t align_; /* Start sr.payload on an 8-byte alignment */
>          struct nbd_structured_reply_offset_data offset_data;
>          struct nbd_structured_reply_offset_hole offset_hole;
>          struct {
>            struct nbd_structured_reply_error error;
>            char msg[NBD_MAX_STRING]; /* Common to all error types */
>            uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */
> -        } __attribute__ ((packed)) error;
> +        } NBD_ATTRIBUTE_PACKED error;
>        } payload;
> -    }  __attribute__ ((packed)) sr;
> +    } sr;
>      uint16_t gflags;
>      uint32_t cflags;
>      uint32_t len;
Seems reasonable.  I didn't know about the uint64_t trick before.

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 5/31/23 04:10, Eric Blake wrote:
> While analyzing 'union sbuf' in preparation to add more members to
the
> union, I noticed several things related to __attribute__((packed))
> that can be improved.  It helps to note that that the bulk of the
> members of 'union sbuf' are already marked packed structures from
> nbd-protocol.h, where we don't need to repeat that annotation in
> internal.h but where it does factor into sbuf alignment.
> 
> First, rather than open-coding the attribute name in internal.h (in
> some places with inconsistent whitespace), we can reuse
> NBD_ATTRIBUTE_PACKED already present from nbd-protocol.h.
> 
> Second, note that two of the union members are themselves substructs
> with two parts: both sbuf.or and sbuf.sr need distinct storage for a
> header (a packed structure) and a payload (a union of various items;
> currently each member of both of those unions are already packed),
> where the choice of union branch and overall size of the payload to be
> read (if any) is determined by information in the header.  Later
> states then refer to information from both the header and payload, so
> we need to keep the sub-structs (rather than hoisting the two parts of
> the sub-struct into being directly-overlapping top-level members of
> union sbuf proper).  But because we don't know the payload size until
> after the header is read, the state machine uses separate recv() calls
> into the two parts of sbuf.or and sbuf.sr; and there is no specific
> reason that the two reads need to occur into adjacent memory.  Thus,
> these two packed annotations buy us nothing at this layer and can be
> safely elided.
> 
> Finally, there are benefits to naturally aligning uint64_t members,
> whether or not hardware supports unaligned access.  Even though we are
> using attribute packed to match wire format (and some NBD messages do
> not have any natural alignment), the judicious use of a non-packed
> uint64_t member to various unions gives the compiler permission to
> insert padding that is otherwise not possible when a packed struct
> occurs immediately before a union containing only packed members.  In
> particular, with structured replies, it is worth ensuring that
> sbuf.sr.payload.offset_data.offset falls on a 64-bit alignment.
> 
> Note that sbuf.or does not need this latter treatment (currently,
> (sbuf.or.payload.export.exportsize is the only uint64_t in that
> particular payload union, but does not occur at a natural alignment to
> begin with); but it is also worth remembering that option negotiation
> is not in the hot path the way sbuf.sr is.
> 
> Actually testing the alignment change from adding align_ members is
> harder to do, especially without C11's <stdalign.h>, in part
because
> on 32-bit platforms where uint64_t is only 4-byte aligned (without gcc
> -malign-double), there is no change to layout.  But in my
> investigation under gdb on x86_64, inserting the otherwise-unused
> align_* members changed both offsetof(struct nbd_handle, sbuf) % 8 and
> (offsetof(struct nbd_handle, sbuf.sr.payload) - offsetof(struct
> nbd_handle, sbuf)) % 8 from 4 to 0, which is what I wanted.
> 
> Not touched here: gcc's -Wpacked says several (but not all) of our
> structures in that file are already naturally aligned, where adding a
> packed notation can actually cause slower code when embedding that
> type in a larger structure (exactly what we're doing when combining
> those structs into sbuf), when the compiler has to prepare for
> unaligned access even if the struct would otherwise be aligned.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> Fallout from review of the 64-bit v3 2/22 patch.
> 
>  lib/internal.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index b155681d..a8c49e16 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -212,6 +212,7 @@ struct nbd_handle {
>     * and commands.
>     */
>    union {
> +    uint64_t align_; /* Start sbuf on an 8-byte alignment where useful */
>      struct nbd_old_handshake old_handshake;
>      struct nbd_new_handshake new_handshake;
>      struct nbd_new_option option;
> @@ -221,34 +222,35 @@ struct nbd_handle {
>          struct {
>            struct nbd_fixed_new_option_reply_server server;
>            char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */
> -        } __attribute__ ((packed)) server;
> +        } NBD_ATTRIBUTE_PACKED server;
>          struct nbd_fixed_new_option_reply_info_export export;
>          struct nbd_fixed_new_option_reply_info_block_size block_size;
>          struct {
>            struct nbd_fixed_new_option_reply_info_name_or_desc info;
>            char str[NBD_MAX_STRING];
> -        } __attribute__ ((packed)) name_desc;
> +        } NBD_ATTRIBUTE_PACKED name_desc;
>          struct {
>            struct nbd_fixed_new_option_reply_meta_context context;
>            char str[NBD_MAX_STRING];
> -        }  __attribute__ ((packed)) context;
> +        } NBD_ATTRIBUTE_PACKED context;
>          char err_msg[NBD_MAX_STRING];
>        } payload;
> -    }  __attribute__ ((packed)) or;
> +    } or;
>      struct nbd_export_name_option_reply export_name_reply;
>      struct nbd_simple_reply simple_reply;
>      struct {
>        struct nbd_structured_reply structured_reply;
>        union {
> +        uint64_t align_; /* Start sr.payload on an 8-byte alignment */
>          struct nbd_structured_reply_offset_data offset_data;
>          struct nbd_structured_reply_offset_hole offset_hole;
>          struct {
>            struct nbd_structured_reply_error error;
>            char msg[NBD_MAX_STRING]; /* Common to all error types */
>            uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */
> -        } __attribute__ ((packed)) error;
> +        } NBD_ATTRIBUTE_PACKED error;
>        } payload;
> -    }  __attribute__ ((packed)) sr;
> +    } sr;
>      uint16_t gflags;
>      uint32_t cflags;
>      uint32_t len;
In my opinion, this should have been three patches:

(1) replace "__attribute__ ((packed)" with NBD_ATTRIBUTE_PACKED,

(2) eliminate the packing for "sr" and "or",

(3) introduce the "align_" fields.

This structuring is actually perfectly reflected by the commit message
(compare "first", "second", "finally"). All these
concepts are mutually
independent, so squashing them together makes for a needlessly
complicated commit message and patch body. I make an honest effort to
internalize every detail of the commit message before starting to read
the code, and lumping together unrelated concepts does not help with
that -- I get overloaded for no good reason.

That said:

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Thanks,
Laszlo