Re: [Qemu-devel] [RFC PATCH 3/4] block: VHDX block driver support

[Jeff Cody]

On Tue, Feb 19, 2013 at 03:30:10PM +0100, Kevin Wolf wrote:
> On Tue, Feb 19, 2013 at 08:26:35AM -0500, Jeff Cody wrote:
> > On Tue, Feb 19, 2013 at 10:20:40AM +0100, Stefan Hajnoczi wrote:
> > > On Mon, Feb 18, 2013 at 06:03:31PM -0500, Jeff Cody wrote:
> > > > +#include "qemu/crc32c.h"
> > > > +#include "block/vhdx.h"
> > > > +
> > > > +#define vhdx_nop(x) do { (void)(x); } while (0)
> > > > +
> > > > +/* Help macros to copy data from file buffers to header
> > > > + * structures, with proper endianness.  These help avoid
> > > > + * using packed structs */
> > > 
> > > Why not use packed structs?  Then you don't need memcpy/endianness
> > > macros.  Abusing *_to_cpu() to go both "to" and "from" CPU endianness
> > > is ugly.
> > 
> > Packed structs are (IMO) worse, and I think should be avoided wherever
> > possible.  Depending on the platform and structure, it can lead to
> > unaligned data access, which is either just slower (x86) or worse
> > (e.g. some arm variants).  Maybe these are all defined well in the
> > spec, so as to be mostly alignment proof from most architectures, but
> > I figured it just as well to avoid their usage since we have to touch
> > each field for alignment (well, I guess only on BE architectures in
> > the end).
> 
> The whole point of packed structs is to enable the unaligned data
> access, so yes, you're right with that. But in my opinion declaring the
> struct packed and letting the compiler deal with unaligned data is
> clearly superior to doing it manually. I mean, this is the kind of
> things that compilers are for.

I have no doubt the compiler can deal with the unalignment.  But we
can do things the compiler essentially can't, when it comes to data
layout.  Maybe I am engaging in too much micro-optimization here, but
the way the compiler has to deal with it is by additional
instructions, since it can't do anything about the data structure
itself.  

By doing it manually (which is nothing magic, it is just
copying struct elements over) we can deal with it on a data structure
level, which should then always yield aligned access without extra
instructions to deal with unalignment.  Now, whether this results in
any real-world performance difference is of course open for debate.


That said, since both of the block maintainers prefer packed structs,
I will change over to those (sigh - and I was so happy with those
macros! :) ).

Thanks,
Jeff