Обсуждение: Bring atomic flag fallback up to snuff

Hi,

As Daniel pointed out in:
https://postgr.es/m/FB948276-7B32-4B77-83E6-D00167F8EEB4@yesql.se the
pg_atomic_flag fallback implementation is broken.  That has gone
unnoticed because the fallback implementation wasn't testable until now:
-   /* ---
-    * Can't run the test under the semaphore emulation, it doesn't handle
-    * checking two edge cases well:
-    * - pg_atomic_unlocked_test_flag() always returns true
-    * - locking a already locked flag blocks
-    * it seems better to not test the semaphore fallback here, than weaken
-    * the checks for the other cases. The semaphore code will be the same
-    * everywhere, whereas the efficient implementations wont.
-    * ---
-    */
and flags weren't used until recently.

The attached fixes the bug and removes the edge-cases by storing a value
separate from the semaphore. I should have done that from the start.
This is an ABI break, but given the fallback didn't work at all, I don't
think that's a problem for backporting.

Fix attached. Comments?

Greetings,

Andres Freund

Hi,

On 2018-04-07 12:57:53 +0000, Magnus Hagander wrote:
> This is the same one you already committed right? Not a further one on top
> of that?

Yes, I pushed a few hours later.


> That said,+1 for not bothering to  back patch it.

I did ;). I was more wondering about whether to backpatch the ABI break
or not, and decided it doesn't matter because all th broken cases were
broken already. And nobody uses the fallback "naturally" anyway.



- Andres

Andres Freund <andres@anarazel.de> writes:
> As Daniel pointed out in:
> https://postgr.es/m/FB948276-7B32-4B77-83E6-D00167F8EEB4@yesql.se the
> pg_atomic_flag fallback implementation is broken.  That has gone
> unnoticed because the fallback implementation wasn't testable until now:
> ...
> The attached fixes the bug and removes the edge-cases by storing a value
> separate from the semaphore. I should have done that from the start.
> This is an ABI break, but given the fallback didn't work at all, I don't
> think that's a problem for backporting.

> Fix attached. Comments?

pademelon says it's wrong.

2018-04-07 13:39:34.982 EDT [1197:89] pg_regress/lock LOG:  statement: SELECT test_atomic_ops();
TRAP: UnalignedPointer("(((uintptr_t) ((uintptr_t)(ptr)) + ((sizeof(*ptr)) - 1)) & ~((uintptr_t) ((sizeof(*ptr)) - 1)))
!=(uintptr_t)(ptr)", File: "../../../src/include/port/atomics.h", Line: 177) 


            regards, tom lane

On 2018-04-07 14:07:05 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > As Daniel pointed out in:
> > https://postgr.es/m/FB948276-7B32-4B77-83E6-D00167F8EEB4@yesql.se the
> > pg_atomic_flag fallback implementation is broken.  That has gone
> > unnoticed because the fallback implementation wasn't testable until now:
> > ...
> > The attached fixes the bug and removes the edge-cases by storing a value
> > separate from the semaphore. I should have done that from the start.
> > This is an ABI break, but given the fallback didn't work at all, I don't
> > think that's a problem for backporting.
> 
> > Fix attached. Comments?
> 
> pademelon says it's wrong.
> 
> 2018-04-07 13:39:34.982 EDT [1197:89] pg_regress/lock LOG:  statement: SELECT test_atomic_ops();
> TRAP: UnalignedPointer("(((uintptr_t) ((uintptr_t)(ptr)) + ((sizeof(*ptr)) - 1)) & ~((uintptr_t) ((sizeof(*ptr)) -
1)))!= (uintptr_t)(ptr)", File: "../../../src/include/port/atomics.h", Line: 177)
 

Yea, I just saw that.

Afaict it's "just" an over-eager / wrong assert. I can't for the heck of
it think why I wrote (9.5 timeframe)
    AssertPointerAlignment(ptr, sizeof(*ptr));
where the bigger ones all have asserts alignment to their own size.  I
assume I did because some platforms want to do atomics bigger than a
single int - but then I should've used sizeof(ptr->value).  So far
pademelon is the only animal affected afaict - let me think about it for
a bit and come up with a patch, ok?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2018-04-07 14:07:05 -0400, Tom Lane wrote:
>> TRAP: UnalignedPointer("(((uintptr_t) ((uintptr_t)(ptr)) + ((sizeof(*ptr)) - 1)) & ~((uintptr_t) ((sizeof(*ptr)) -
1)))!= (uintptr_t)(ptr)", File: "../../../src/include/port/atomics.h", Line: 177) 

> Yea, I just saw that.

> Afaict it's "just" an over-eager / wrong assert. I can't for the heck of
> it think why I wrote (9.5 timeframe)
>     AssertPointerAlignment(ptr, sizeof(*ptr));
> where the bigger ones all have asserts alignment to their own size.  I
> assume I did because some platforms want to do atomics bigger than a
> single int - but then I should've used sizeof(ptr->value).  So far
> pademelon is the only animal affected afaict - let me think about it for
> a bit and come up with a patch, ok?

I think I'd just drop those asserts altogether.  The hardware is in charge
of complaining about misaligned pointers.

If you do insist on asserting something, it needs to be about ptr->sema;
the bool value field isn't going to have any interesting alignment
requirement, but the sema might.

            regards, tom lane

Hi,

On 2018-04-07 14:23:38 -0400, Tom Lane wrote:
> I think I'd just drop those asserts altogether.  The hardware is in charge
> of complaining about misaligned pointers.

Well, the problem is that some atomics operations on some platforms do
not fail for unaligned pointers, they just loose their atomic
property. Fun times.


> If you do insist on asserting something, it needs to be about ptr->sema;
> the bool value field isn't going to have any interesting alignment
> requirement, but the sema might.

The alignment requirements of sema is going to be taken care of by
normal alignment rules, otherwise we'd be in trouble all over the tree
already.

Think I'll just drop it from the general branch, and add it to the
generic gcc implementation that uses a four byte value (because of arm
and the like).

Greetings,

Andres Freund

Hi,

On 2018-04-07 14:23:38 -0400, Tom Lane wrote:
> I think I'd just drop those asserts altogether.  The hardware is in charge
> of complaining about misaligned pointers.

I came around to that view. The problem with "natural" and atomic
alignment differing is only relevant for 64bit integers (4 byte natural,
8 byte atomic).  Pushing that once the tests finish.

Greetings,

Andres Freund