[Libguestfs] [libnbd PATCH] internal: Tweak use of attribute packed in union sbuf
Laszlo Ersek
lersek at redhat.com
Wed May 31 10:45:16 UTC 2023
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
More information about the Libguestfs
mailing list