Re: glibc 2.21 - Machine maintainers, please test your machines.

[Chris Metcalf <cmetcalf at ezchip dot com>]

On 1/26/2015 3:55 PM, Torvald Riegel wrote:
> On Mon, 2015-01-26 at 15:21 -0500, Chris Metcalf wrote:
> > On 1/26/2015 7:44 AM, Torvald Riegel wrote:
> > > > +         will have the necessary alignment in this case, so we are
> > > > +         not obliged to use the to_new_sem macro in this case.  */
> > > I think we should just use to_new_sem here too for consistency, but
> > > change the comment to point out that the address to which we will copy
> > > newsem via the file write and mmap later will have a compatible
> > > alignment with the natural alignment of NEWSEM.
> > The diff to make it look like that is more complicated, so I resisted doing
> > it, particularly late in the freeze.
> >
> > All that said, I'm happy to do v2 that way, and you can see what you think.
> > I don't actually think it's necessary to explain that to_new_sem() is a known
> > no-op in this case; we're using it just to be consistent anyway.
> Sorry, what I wrote was misleading, and incomplete:  I still think using
> to_new_sem would be consistent -- but if we do that, we need to force
> the same alignment as we will have when effectively copy it via file
> write and mmap later on.  I don't think the current code does that.

Yes, I understand your point now.  So I believe to accomplish that we need:

      sem_t sem __attribute__((aligned(__alignof__(struct new_sem))));;
      struct new_sem *newsem = to_new_sem (&sem);

Is this cleaner then the previous code with the union?  I guess arguably
so, though as you say we still need a comment explaining that we are
aligning the sem_t object to match its eventual page-alignment after
it is written out to a file and used by mmap.


> If we don't use to_new_sem as you had in your patch, then we don't need
> to force the alignment -- but in this case we have to make sure that the
> code behaves as if to_new_sem didn't adapt the offset of new_sem within
> sem_t (which your previous code made sure by using the union).

Yes. this was the previous behavior.  It was correct but needed more comments
to explain how it was copied to a file with offset zero mod eight.

> Nonetheless, I think that then, we need to point out that we must not
> use to_new_sem, so that it doesn't assume a wrong alignment (based on
> sem).

Well, to_new_sem() would be OK in the union case, since the sem_t would
have alignment zero mod eight due to the struct new_sem sharing the
alignment of the overall union.  But it doesn't really make sense to use
to_new_sem() and the union; it feels either/or.  We should just choose one.

More importantly, we still need to really confirm that the to_new_sem()
approach makes sense as the best overall strategy.

For tilegx32, we need either this or just using 32-bit atomics.  For x32,
we can keep using what we have now for them, or they can use this new
aligning code and presumably get somewhat faster semaphores.  I don't
know for m68k and mips what they need.

For aarch64 in ILP32 mode, or other new ILP32 architectures with 64-bit
atomics, it probably makes the most sense to force sem_t to be 8-byte
aligned and not have to deal with any of this.  (I note that aarch64 also
does not support unaligned atomics, so it needs to do something, one
way or the other.)

--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com