[PATCH] elf: Use C11 like atomics on _dl_mcount

[PATCH] elf: Use C11 like atomics on _dl_mcount

[Wilco Dijkstra]

Hi Adhemerval,

-	      newfromidx = catomic_exchange_and_add (&fromidx, 1) + 1;
+	      newfromidx = atomic_fetch_and_acquire (&fromidx, 1) + 1;

I don't see how that could possibly work...

Btw is this trying to split my catomic patch into baby steps? Given these
are simple counters (not locks), you only need atomicity, so relaxed MO
is the obvious equivalent.

Cheers,
Wilco

Re: [PATCH] elf: Use C11 like atomics on _dl_mcount

[Adhemerval Zanella Netto]

On 01/09/22 09:21, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
> -	      newfromidx = catomic_exchange_and_add (&fromidx, 1) + 1;
> +	      newfromidx = atomic_fetch_and_acquire (&fromidx, 1) + 1;
> 
> I don't see how that could possibly work...
> 
> Btw is this trying to split my catomic patch into baby steps? Given these
> are simple counters (not locks), you only need atomicity, so relaxed MO
> is the obvious equivalent.

Right, I was not trying to be clever here and acquire might be a more
strict memory order.

And I haven't replied yet to you atomic refactor, but I also think changing
step by step is indeed better than changing all the atomic altogether.

Do you think using relaxed on all atomic is suffice here?

Re: [PATCH] elf: Use C11 like atomics on _dl_mcount

[Wilco Dijkstra]

Hi Adhemerval,

> Right, I was not trying to be clever here and acquire might be a more
> strict memory order.
>
> And I haven't replied yet to you atomic refactor, but I also think changing
> step by step is indeed better than changing all the atomic altogether.

Well that's what I did, I split into about 15-20 patches rather than a single one.

> Do you think using relaxed on all atomic is suffice here?

Indeed, adding acquire on these counters will not ensure correct memory ordering.

Note this particular code looks very unsafe - it is not clear to me how it blocks
other threads from accessing new elements before they are properly initialized.
Also the counters that are being updated atomically are read without atomic
accesses, so may get a cached value or something newer - you don't know.

Cheers,
Wilco

Re: [PATCH] elf: Use C11 like atomics on _dl_mcount

[Adhemerval Zanella Netto]

On 01/09/22 11:19, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
>> Right, I was not trying to be clever here and acquire might be a more
>> strict memory order.
>>
>> And I haven't replied yet to you atomic refactor, but I also think changing
>> step by step is indeed better than changing all the atomic altogether.
> 
> Well that's what I did, I split into about 15-20 patches rather than a single one.

Is the https://patchwork.sourceware.org/project/glibc/list/?series=10727 right? I 
think it has failed CI, could you submit again?

> 
>> Do you think using relaxed on all atomic is suffice here?
> 
> Indeed, adding acquire on these counters will not ensure correct memory ordering.
> 
> Note this particular code looks very unsafe - it is not clear to me how it blocks
> other threads from accessing new elements before they are properly initialized.
> Also the counters that are being updated atomically are read without atomic
> accesses, so may get a cached value or something newer - you don't know.
> 

I thinking we are tracking two different issues, although they are correlated.
My take is to first try to map the current code to a C11-like atomic, so we
can always use compiler builtins where applicable (there are couple of 
architecture that we might avoid using libgcc for performance reasons), and
then check if the atomic usage does make sense.

Re: [PATCH] elf: Use C11 like atomics on _dl_mcount

[Wilco Dijkstra]

Hi Adhemerval,

> Is the https://patchwork.sourceware.org/project/glibc/list/?series=10727 right? I 
> think it has failed CI, could you submit again?

I'll rebase it. I'll probably move the files that cause all the conflicts into a different
patch (an alternative would be to increase the vertical spacing between all the
defines first so that merge tools no longer get confused).

> I thinking we are tracking two different issues, although they are correlated.
> My take is to first try to map the current code to a C11-like atomic, so we
> can always use compiler builtins where applicable (there are couple of 
> architecture that we might avoid using libgcc for performance reasons), and
> then check if the atomic usage does make sense.

Yes, so I first mapped them to an atomic with the most appropriate MO (since
the MO of many existing macros is not well defined). We should look at
correctness in a second pass since it is already clear there are many concurrency
bugs.

Cheers,
Wilco

Re: [PATCH] elf: Use C11 like atomics on _dl_mcount

[Adhemerval Zanella Netto]

On 01/09/22 20:18, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
>> Is the https://patchwork.sourceware.org/project/glibc/list/?series=10727 right? I 
>> think it has failed CI, could you submit again?
> 
> I'll rebase it. I'll probably move the files that cause all the conflicts into a different
> patch (an alternative would be to increase the vertical spacing between all the
> defines first so that merge tools no longer get confused).
> 
>> I thinking we are tracking two different issues, although they are correlated.
>> My take is to first try to map the current code to a C11-like atomic, so we
>> can always use compiler builtins where applicable (there are couple of 
>> architecture that we might avoid using libgcc for performance reasons), and
>> then check if the atomic usage does make sense.
> 
> Yes, so I first mapped them to an atomic with the most appropriate MO (since
> the MO of many existing macros is not well defined). We should look at
> correctness in a second pass since it is already clear there are many concurrency
> bugs.

And it seems that for 10727 series you didn't change the elf/dl-profile.c.  With
the relaxed MO for the counters, are you ok with the change?

Re: [PATCH] elf: Use C11 like atomics on _dl_mcount

[Wilco Dijkstra]

Hi Adhemerval,

> And it seems that for 10727 series you didn't change the elf/dl-profile.c.  With
> the relaxed MO for the counters, are you ok with the change?

Yes that's fine, but please use ADD rather than AND. It's also a good idea to
add a comment that the function is not MT safe.

Cheers,
Wilco

[PATCH] elf: Use C11 like atomics on _dl_mcount

[Adhemerval Zanella]

Checked on x86_64-linux-gnu.
---
 elf/dl-profile.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/elf/dl-profile.c b/elf/dl-profile.c
index ec57e3a965..06dbeec4b1 100644
--- a/elf/dl-profile.c
+++ b/elf/dl-profile.c
@@ -548,7 +548,7 @@ _dl_mcount (ElfW(Addr) frompc, ElfW(Addr) selfpc)
 	      size_t newfromidx;
 	      to_index = (data[narcs].self_pc
 			  / (HASHFRACTION * sizeof (*tos)));
-	      newfromidx = catomic_exchange_and_add (&fromidx, 1) + 1;
+	      newfromidx = atomic_fetch_and_acquire (&fromidx, 1) + 1;
 	      froms[newfromidx].here = &data[narcs];
 	      froms[newfromidx].link = tos[to_index];
 	      tos[to_index] = newfromidx;
@@ -558,14 +558,14 @@ _dl_mcount (ElfW(Addr) frompc, ElfW(Addr) selfpc)
 	  /* If we still have no entry stop searching and insert.  */
 	  if (*topcindex == 0)
 	    {
-	      unsigned int newarc = catomic_exchange_and_add (narcsp, 1);
+	      unsigned int newarc = atomic_exchange_and_add (narcsp, 1) + 1;
 
 	      /* In rare cases it could happen that all entries in FROMS are
 		 occupied.  So we cannot count this anymore.  */
 	      if (newarc >= fromlimit)
 		goto done;
 
-	      *topcindex = catomic_exchange_and_add (&fromidx, 1) + 1;
+	      *topcindex = atomic_fetch_and_acquire (&fromidx, 1) + 1;
 	      fromp = &froms[*topcindex];
 
 	      fromp->here = &data[newarc];
@@ -573,7 +573,7 @@ _dl_mcount (ElfW(Addr) frompc, ElfW(Addr) selfpc)
 	      data[newarc].self_pc = selfpc;
 	      data[newarc].count = 0;
 	      fromp->link = 0;
-	      catomic_increment (&narcs);
+	      atomic_fetch_and_acquire (&narcs, 1);
 
 	      break;
 	    }
@@ -586,7 +586,7 @@ _dl_mcount (ElfW(Addr) frompc, ElfW(Addr) selfpc)
     }
 
   /* Increment the counter.  */
-  catomic_increment (&fromp->here->count);
+  atomic_fetch_and_acquire (&fromp->here->count, 1);
 
  done:
   ;
-- 
2.34.1