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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Jan 26, 2015 at 3:29 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote:
> 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.

What is the drawback with 32-bit atomics for x32? If there is no real
advantage for 64-bit atomics, x32 will use 32-bit atomics.  If x32 needs
to use 64-bit atomics, I need to investigate which way to go.

-- 
H.J.