[RFC] Alignment- / padding-requirements

[Swen Schillig]

Hi Martin
On Thu, 2018-03-15 at 16:20 +1100, Martin Schwenke via samba-technical
wrote:
> On Thu, 15 Mar 2018 11:29:23 +1100, Martin Schwenke via samba-
> technical
> <samba-technical at lists.samba.org> wrote:
> 
> > On Mon, 05 Mar 2018 09:42:15 +0100, Swen Schillig via samba-
> > technical
> > <samba-technical at lists.samba.org> wrote:
> > 
> > > For the last few days I've been looking at 
> > > the memory handing within CTDB and I stumbled over code snippets
> > > like
> > > 
> > >         size = (length+(CTDB_DS_ALIGNMENT-1)) &
> > > ~(CTDB_DS_ALIGNMENT-1);
> > > 
> > > I might be wrong, but what is done here is padding and not
> > > alignment
> > > and I might be wrong again, but padding memory to a multiple of
> > > the
> > > wanted alignment does not guarantee the alignment.
> > > So, I'm a bit puzzled what is really wanted here.
> > > 
> > > From the comments around the define 
> > > 
> > > /* we must align packets to ensure ctdb works on all
> > > architectures (eg.
> > > sparc) */
> > > #define CTDB_DS_ALIGNMENT 8
> > > 
> > > .. I take it that really alignment is required to make sure 64bit
> > > access is 
> > > guaranteed on architectures which need it (here SPARC !?)
> > > 
> > > But the structure(s) which are wanted to be aligned 
> > >         struct ctdb_req_call_old
> > >         struct ctdb_req_header 
> > > do not contain any 64bit values.
> > > 
> > > So here comes my question, do we really need to do those ugly
> > > padding
> > > tricks and  
> > 
> > After talking with people who are smarter than me, I finally have a
> > theory for this!  :-)
> > 
> > 1. malloc() returns 8-byte aligned memory (according to POSIX, the
> > C
> >    standard, malloc(3))
> > 
> > 2. CTDB's packet allocation code pads out to a multiple of 8 bytes
> > 
> > 3. CTDB writes the padded packets, each starting with a header, out
> > to
> >    a socket
> > 
> > 4. As the receiving CTDB reads data from a socket, it might read
> > the
> >    data for the current packet and then the beginning of the next
> >    packet into a buffer
> > 
> >    Padding each packet when sending guarantees that the header
> >    at beginning of the next packet is always aligned in the buffer
> > on
> >    the receiving side.
> > 
> > So, the padding is required to guarantee alignment of subsequent
> > headers
> > because we're reading the header out of the buffer as a struct.  If
> > we
> > were reading and unmarshalling from the buffer as a byte stream
> > then we
> > wouldn't need to do this.
> > 
> > This theory can (probably) be tested.  You could add an assert() to
> > the
> > reading code to ensure that each header is aligned.  Then remove
> > the
> > "alignment" code and see if you trip the assert().
> > 
> > Someone please yell if I've got this horribly wrong!
> 
> Note that this theory doesn't apply in the current ctdb_io.c code
> because it does a memmove() to move residual data to the front of the
> buffer.  The previous version seems to have only read up until the
> end
> of the packet.  Not sure about previous versions.
> 
> However, this might not be a useful theory after all...
> 
> peace & happiness,
> martin

Thank you very much for your answers, efforts and thoughts.
It's, as always very well appreciated.

Assuming that your theory is correct, the naming should more be PADD as
opposed to ALIGN, right ?

Another, more important thing.
We know that the padding is NOT required for any functional reason,
so with this ALIGN/PADD code we're saying that it is more
beneficial(speed ?) to us, to constantly re-allocate / move buffers
than just using them as they come, correct ?
Not to forget about the extra-/NULL-data which must go over the wire !

Even though I seriously doubt that, I guess I will need to prove it :-)

Besides, on the receiving side we "copy" the data into our structures,
which are aligned by nature, as you pointed out correctly.
So, I'm not really sure where this padding is supposed to help.

Anyways, any other/additional thoughts on this ?

Cheers Swen