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

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

On Wed, May 31, 2023 at 12:45:16PM +0200, Laszlo Ersek
wrote:
> 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,...
> > 
> > Second,...
> > 
> > Finally, there are benefits to naturally aligning uint64_t members,...
> 
> 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.
Concur, although I swapped (2) and (1) to minimize churn.
> 
> 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>
I've split the patch then added your R-b (the end result to code is
the same, and I think the commit messages are better); now in as
commits c1df4df9..03de4514

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org