Re: [Libguestfs] [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(a)redhat.com&gt; --- 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;