public inbox for libc-help@sourceware.org
 help / color / mirror / Atom feed
* rwlock different behavior on glibc 2.28 then on previous versions
@ 2022-03-07 20:13 David Mozes
  2022-03-11 13:58 ` Florian Weimer
  0 siblings, 1 reply; 10+ messages in thread
From: David Mozes @ 2022-03-07 20:13 UTC (permalink / raw)
  To: libc-help

I like to consult with you regarding the flowing issue

The rwlock rewrite commit:

commit cc25c8b4c1196a8c29e9a45b1e096b99a87b7f8c
Author: Torvald Riegel <triegel@redhat.com>
Date:   Thu May 22 16:00:12 2014 +0200

changed the behavior of the rwlock as well!

On the previous version of Glibc (before the commit) We could release write lock with different thread as the one that owned the lock (If we like) . This is help very much for async program that has a many threads that communicate with some target.
And we like to define a call back function instead of blocking on aio thread that send the data in order to release the rwlock.this improves the performance dramatically .
For backward compatibility and better performance  I suggest a small change as describe below:
Remove the THREAD_SELF checking(See below) ,unless this change is danger!

Does such fix is danger ?
Do you aware why this check was added in the first place?


Other option is to define anther function /Type that enable less restricted operation to not violate the API agreement and it will be on the user responsibly whether to use the new type or not for better performance.
What do you think?

Thx
David



The  previous code before  cc25c8b4c1196a8c29e9a45b1e096b99a87b7f8c was look like that:
__pthread_rwlock_unlock (pthread_rwlock_t *rwlock)
{
  lll_lock (rwlock->__data.__lock, rwlock->__data.__shared);
  if (rwlock->__data.__writer)
    rwlock->__data.__writer = 0;
  else
    --rwlock->__data.__nr_readers;
  if (rwlock->__data.__nr_readers == 0)
    {
      if (rwlock->__data.__nr_writers_queued)
        {

Our application has many inflight IO threads


After the cc25c8b4c1196a8c29e9a45b1e096b99a87b7f8c commit:

     int
___pthread_rwlock_unlock (pthread_rwlock_t *rwlock)
{
  LIBC_PROBE (rwlock_unlock, 1, rwlock);

  /* We distinguish between having acquired a read vs. a write lock by looking
     at the writer TID.  If it's equal to our TID, we must be the writer
     because nobody else can have stored this value.  Also, if we are a
     reader, we will read from the wrunlock store with value 0 by the most
     recent writer because that writer happens-before us.  */
  if (atomic_load_relaxed (&rwlock->__data.__cur_writer)
      == THREAD_GETMEM (THREAD_SELF, tid))
      __pthread_rwlock_wrunlock (rwlock);
  else
    __pthread_rwlock_rdunlock (rwlock);
  return 0;
}





The change I propose is :


What do you think regarding such a small fix  like this:

   if (atomic_load_relaxed (&rwlock->__data.__cur_writer))
      __pthread_rwlock_wrunlock (rwlock);
  else
    __pthread_rwlock_rdunlock (rwlock);
  return 0;
}



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-07 20:13 rwlock different behavior on glibc 2.28 then on previous versions David Mozes
@ 2022-03-11 13:58 ` Florian Weimer
  2022-03-13 10:23   ` David Mozes
  0 siblings, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2022-03-11 13:58 UTC (permalink / raw)
  To: David Mozes; +Cc: libc-help

* David Mozes:

> On the previous version of Glibc (before the commit) We could release
> write lock with different thread as the one that owned the lock (If we
> like) .

Sorry, this misuse of the API has always been undefined according to
POSIX (“Results are undefined if the read-write lock rwlock is not held
by the calling thread.”).

> This is help very much for async program that has a many threads that
> communicate with some target.  And we like to define a call back
> function instead of blocking on aio thread that send the data in order
> to release the rwlock.this improves the performance dramatically .

Could you use a different synchronization object (perhaps a condition
variable)?

Thanks,
Florian


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-11 13:58 ` Florian Weimer
@ 2022-03-13 10:23   ` David Mozes
  2022-03-20 16:44     ` David Mozes
  0 siblings, 1 reply; 10+ messages in thread
From: David Mozes @ 2022-03-13 10:23 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-help

Thx Florin,


-----Original Message-----
From: Florian Weimer <fweimer@redhat.com> 
Sent: Friday, March 11, 2022 3:59 PM
To: David Mozes <david.mozes@silk.us>
Cc: libc-help@sourceware.org
Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions

* David Mozes:

> On the previous version of Glibc (before the commit) We could release
> write lock with different thread as the one that owned the lock (If we
> like) .

Sorry, this misuse of the API has always been undefined according to
POSIX (“Results are undefined if the read-write lock rwlock is not held
by the calling thread.”).

 >> Correct, but practically we can do it on the user's responsibility.  
>>Do you know if this "if" solved some synchronization issue or just enforced the undefined  API? 

> This is help very much for async program that has a many threads that
> communicate with some target.  And we like to define a call back
> function instead of blocking on aio thread that send the data in order
> to release the rwlock.this improves the performance dramatically .

Could you use a different synchronization object (perhaps a condition
variable)?

>> If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states. 
>>If we like to keep the API agreement, why not define the different async programming unlock functions? 
Thanks
Florian

Thx
David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-13 10:23   ` David Mozes
@ 2022-03-20 16:44     ` David Mozes
  2022-03-21  6:19       ` Christian Hoff
  0 siblings, 1 reply; 10+ messages in thread
From: David Mozes @ 2022-03-20 16:44 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-help

Hi,
Where are we with this? 
Also, the current implementation of the __pthread_rwlock_unlock  is problematic.
On the one hand, it checks the API agreement. However, on the case of violation, the user don't have any induction 
And function return OK 
See example code below:


struct threadInfo{
    pthread_t* tid;
    pthread_rwlock_t *rwlock;
};

void *lockerThreadFunction(void *vargp)
{
    km_status stat = KM_OK;
    struct threadInfo* Tinfo = (struct threadInfo*)vargp;

    do {
        stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
        if (KM_OK == stat) {
            printf("[INFO] - FIRST thread 0x%x locked %p \n", *(Tinfo->tid), Tinfo->rwlock);
            sleep(10);
            stat = pthread_rwlock_unlock(Tinfo->rwlock);
            if (KM_OK == stat) {
                printf("[INFO] FIRST thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
                sleep(1);
            }
        }
    }while(KM_OK != stat);
    return NULL;
}


void *secondThreadFunction(void *vargp)
{
    km_status stat = KM_OK;
    struct threadInfo* Tinfo = (struct threadInfo*)vargp;

    sleep(5);
    do {
        stat = pthread_rwlock_unlock(Tinfo->rwlock);
        if (KM_OK == stat) {
            printf("[INFO] SECOND thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
            sleep(1);
        } else 
            printf("[INFO] SECOND thread 0x%x try_to_released %p erro status is: %d \n", *(Tinfo->tid), Tinfo->rwlock,stat);
        
    }while(KM_OK != stat);
    return NULL;
}

void *thirdThreadFunction(void *vargp)
{
    km_status stat = KM_OK;
    struct threadInfo* Tinfo = (struct threadInfo*)vargp;

    sleep(2);
    do {
        stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
        if (KM_OK == stat) {
            printf("[INFO] THIRD thread 0X%x locked %x\n", *(Tinfo->tid), Tinfo->rwlock);
            sleep(5);
        }
        else {
            printf("[ERROR]: THIRD thread 0x%x failed Locking %p status is %d\n", *(Tinfo->tid), Tinfo->rwlock,stat);
            sleep(1);
        }
    }while(KM_OK != stat);
    return NULL;
}

void main()
{
    km_status stat = KM_OK;
    /* km_rwlock lock; */
    pthread_rwlock_t rwlock;
    pthread_t lockerTid = 0, releaseTid = 0, thirdTid=0;
    struct threadInfo firstTinfo, secondTinfo, thirdTinfo;

   /* stat = km_rwlock_init(&rwlock, KM_TRUE); */
    stat = pthread_rwlock_init (&rwlock, NULL);

    if (KM_OK == stat) {
        printf("read\\writer lock initialized\n");
        printf("Before Thread\n");
        
        firstTinfo.tid  = &lockerTid;
        firstTinfo.rwlock = &rwlock;
        secondTinfo.tid  = &releaseTid;
        secondTinfo.rwlock = &rwlock;
        thirdTinfo.tid  = &thirdTid;
        thirdTinfo.rwlock = &rwlock;
        

        pthread_create(&lockerTid, NULL, lockerThreadFunction, &firstTinfo);
        pthread_create(&releaseTid,  NULL, secondThreadFunction, &secondTinfo);
        pthread_create(&thirdTid, NULL, thirdThreadFunction, &thirdTinfo);
        printf("After Thread %x\n", lockerTid);

pthread_join(lockerTid, NULL);
        pthread_join(releaseTid, NULL);
        pthread_join(thirdTid, NULL);



the output we saw is this:


the ouput we saw is this:

[root@Rocky85Mozes thread_lock]# ./rw_test
read\writer lock initialized
Before Thread
[INFO] - FIRST thread 0x461f700 locked 0x7ffcda053cc0 
After Thread 461f700
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16  -> first thread take the rwlock
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[INFO] SECOND thread 0x3e1e700 released 0x7ffcda053cc0   -> Second thread unlock the rwlock -- > and get OK!!
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[INFO] FIRST thread 0x461f700 released 0x7ffcda053cc0 
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16


-----Original Message-----
From: David Mozes 
Sent: Sunday, March 13, 2022 12:23 PM
To: Florian Weimer <fweimer@redhat.com>
Cc: libc-help@sourceware.org
Subject: RE: rwlock different behavior on glibc 2.28 then on previous versions

Thx Florin,


-----Original Message-----
From: Florian Weimer <fweimer@redhat.com> 
Sent: Friday, March 11, 2022 3:59 PM
To: David Mozes <david.mozes@silk.us>
Cc: libc-help@sourceware.org
Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions

* David Mozes:

> On the previous version of Glibc (before the commit) We could release
> write lock with different thread as the one that owned the lock (If we
> like) .

Sorry, this misuse of the API has always been undefined according to
POSIX (“Results are undefined if the read-write lock rwlock is not held
by the calling thread.”).

 >> Correct, but practically we can do it on the user's responsibility.  
>>Do you know if this "if" solved some synchronization issue or just enforced the undefined  API? 

> This is help very much for async program that has a many threads that
> communicate with some target.  And we like to define a call back
> function instead of blocking on aio thread that send the data in order
> to release the rwlock.this improves the performance dramatically .

Could you use a different synchronization object (perhaps a condition
variable)?

>> If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states. 
>>If we like to keep the API agreement, why not define the different async programming unlock functions? 
Thanks
Florian

Thx
David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-20 16:44     ` David Mozes
@ 2022-03-21  6:19       ` Christian Hoff
  2022-03-21 14:01         ` David Mozes
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Hoff @ 2022-03-21  6:19 UTC (permalink / raw)
  To: David Mozes, Florian Weimer; +Cc: libc-help

Hallo David,

did you try to run your program with Helgrind already? Helgrind was
designed to catch problems like this. Maybe Thread Sanitizer would also
detect the issue.


Best regards,

    Christian

On 3/20/22 5:44 PM, David Mozes wrote:
> Hi,
> Where are we with this?
> Also, the current implementation of the __pthread_rwlock_unlock  is problematic.
> On the one hand, it checks the API agreement. However, on the case of violation, the user don't have any induction
> And function return OK
> See example code below:
>
>
> struct threadInfo{
>      pthread_t* tid;
>      pthread_rwlock_t *rwlock;
> };
>
> void *lockerThreadFunction(void *vargp)
> {
>      km_status stat = KM_OK;
>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>
>      do {
>          stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>          if (KM_OK == stat) {
>              printf("[INFO] - FIRST thread 0x%x locked %p \n", *(Tinfo->tid), Tinfo->rwlock);
>              sleep(10);
>              stat = pthread_rwlock_unlock(Tinfo->rwlock);
>              if (KM_OK == stat) {
>                  printf("[INFO] FIRST thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>                  sleep(1);
>              }
>          }
>      }while(KM_OK != stat);
>      return NULL;
> }
>
>
> void *secondThreadFunction(void *vargp)
> {
>      km_status stat = KM_OK;
>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>
>      sleep(5);
>      do {
>          stat = pthread_rwlock_unlock(Tinfo->rwlock);
>          if (KM_OK == stat) {
>              printf("[INFO] SECOND thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>              sleep(1);
>          } else
>              printf("[INFO] SECOND thread 0x%x try_to_released %p erro status is: %d \n", *(Tinfo->tid), Tinfo->rwlock,stat);
>
>      }while(KM_OK != stat);
>      return NULL;
> }
>
> void *thirdThreadFunction(void *vargp)
> {
>      km_status stat = KM_OK;
>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>
>      sleep(2);
>      do {
>          stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>          if (KM_OK == stat) {
>              printf("[INFO] THIRD thread 0X%x locked %x\n", *(Tinfo->tid), Tinfo->rwlock);
>              sleep(5);
>          }
>          else {
>              printf("[ERROR]: THIRD thread 0x%x failed Locking %p status is %d\n", *(Tinfo->tid), Tinfo->rwlock,stat);
>              sleep(1);
>          }
>      }while(KM_OK != stat);
>      return NULL;
> }
>
> void main()
> {
>      km_status stat = KM_OK;
>      /* km_rwlock lock; */
>      pthread_rwlock_t rwlock;
>      pthread_t lockerTid = 0, releaseTid = 0, thirdTid=0;
>      struct threadInfo firstTinfo, secondTinfo, thirdTinfo;
>
>     /* stat = km_rwlock_init(&rwlock, KM_TRUE); */
>      stat = pthread_rwlock_init (&rwlock, NULL);
>
>      if (KM_OK == stat) {
>          printf("read\\writer lock initialized\n");
>          printf("Before Thread\n");
>
>          firstTinfo.tid  = &lockerTid;
>          firstTinfo.rwlock = &rwlock;
>          secondTinfo.tid  = &releaseTid;
>          secondTinfo.rwlock = &rwlock;
>          thirdTinfo.tid  = &thirdTid;
>          thirdTinfo.rwlock = &rwlock;
>
>
>          pthread_create(&lockerTid, NULL, lockerThreadFunction, &firstTinfo);
>          pthread_create(&releaseTid,  NULL, secondThreadFunction, &secondTinfo);
>          pthread_create(&thirdTid, NULL, thirdThreadFunction, &thirdTinfo);
>          printf("After Thread %x\n", lockerTid);
>
> pthread_join(lockerTid, NULL);
>          pthread_join(releaseTid, NULL);
>          pthread_join(thirdTid, NULL);
>
>
>
> the output we saw is this:
>
>
> the ouput we saw is this:
>
> [root@Rocky85Mozes thread_lock]# ./rw_test
> read\writer lock initialized
> Before Thread
> [INFO] - FIRST thread 0x461f700 locked 0x7ffcda053cc0
> After Thread 461f700
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16  -> first thread take the rwlock
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [INFO] SECOND thread 0x3e1e700 released 0x7ffcda053cc0   -> Second thread unlock the rwlock -- > and get OK!!
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [INFO] FIRST thread 0x461f700 released 0x7ffcda053cc0
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>
>
> -----Original Message-----
> From: David Mozes
> Sent: Sunday, March 13, 2022 12:23 PM
> To: Florian Weimer <fweimer@redhat.com>
> Cc: libc-help@sourceware.org
> Subject: RE: rwlock different behavior on glibc 2.28 then on previous versions
>
> Thx Florin,
>
>
> -----Original Message-----
> From: Florian Weimer <fweimer@redhat.com>
> Sent: Friday, March 11, 2022 3:59 PM
> To: David Mozes <david.mozes@silk.us>
> Cc: libc-help@sourceware.org
> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
>
> * David Mozes:
>
>> On the previous version of Glibc (before the commit) We could release
>> write lock with different thread as the one that owned the lock (If we
>> like) .
> Sorry, this misuse of the API has always been undefined according to
> POSIX (“Results are undefined if the read-write lock rwlock is not held
> by the calling thread.”).
>
>   >> Correct, but practically we can do it on the user's responsibility.
>>> Do you know if this "if" solved some synchronization issue or just enforced the undefined  API?
>> This is help very much for async program that has a many threads that
>> communicate with some target.  And we like to define a call back
>> function instead of blocking on aio thread that send the data in order
>> to release the rwlock.this improves the performance dramatically .
> Could you use a different synchronization object (perhaps a condition
> variable)?
>
>>> If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states.
>>> If we like to keep the API agreement, why not define the different async programming unlock functions?
> Thanks
> Florian
>
> Thx
> David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-21  6:19       ` Christian Hoff
@ 2022-03-21 14:01         ` David Mozes
  2022-03-21 17:01           ` Adhemerval Zanella
  0 siblings, 1 reply; 10+ messages in thread
From: David Mozes @ 2022-03-21 14:01 UTC (permalink / raw)
  To: Christian Hoff, Florian Weimer; +Cc: libc-help

Hello Christain,
Yes, Thread Sanitizer caught it. 
However, we can't run it all the time on the production code.
We need to make a small change of the unlock code to have async programing and backward compatibility for older code.

Thx
David  
	
-----Original Message-----
From: Christian Hoff <christian_hoff@gmx.net> 
Sent: Monday, March 21, 2022 8:20 AM
To: David Mozes <david.mozes@silk.us>; Florian Weimer <fweimer@redhat.com>
Cc: libc-help@sourceware.org
Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions

Hallo David,

did you try to run your program with Helgrind already? Helgrind was
designed to catch problems like this. Maybe Thread Sanitizer would also
detect the issue.


Best regards,

    Christian

On 3/20/22 5:44 PM, David Mozes wrote:
> Hi,
> Where are we with this?
> Also, the current implementation of the __pthread_rwlock_unlock  is problematic.
> On the one hand, it checks the API agreement. However, on the case of violation, the user don't have any induction
> And function return OK
> See example code below:
>
>
> struct threadInfo{
>      pthread_t* tid;
>      pthread_rwlock_t *rwlock;
> };
>
> void *lockerThreadFunction(void *vargp)
> {
>      km_status stat = KM_OK;
>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>
>      do {
>          stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>          if (KM_OK == stat) {
>              printf("[INFO] - FIRST thread 0x%x locked %p \n", *(Tinfo->tid), Tinfo->rwlock);
>              sleep(10);
>              stat = pthread_rwlock_unlock(Tinfo->rwlock);
>              if (KM_OK == stat) {
>                  printf("[INFO] FIRST thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>                  sleep(1);
>              }
>          }
>      }while(KM_OK != stat);
>      return NULL;
> }
>
>
> void *secondThreadFunction(void *vargp)
> {
>      km_status stat = KM_OK;
>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>
>      sleep(5);
>      do {
>          stat = pthread_rwlock_unlock(Tinfo->rwlock);
>          if (KM_OK == stat) {
>              printf("[INFO] SECOND thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>              sleep(1);
>          } else
>              printf("[INFO] SECOND thread 0x%x try_to_released %p erro status is: %d \n", *(Tinfo->tid), Tinfo->rwlock,stat);
>
>      }while(KM_OK != stat);
>      return NULL;
> }
>
> void *thirdThreadFunction(void *vargp)
> {
>      km_status stat = KM_OK;
>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>
>      sleep(2);
>      do {
>          stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>          if (KM_OK == stat) {
>              printf("[INFO] THIRD thread 0X%x locked %x\n", *(Tinfo->tid), Tinfo->rwlock);
>              sleep(5);
>          }
>          else {
>              printf("[ERROR]: THIRD thread 0x%x failed Locking %p status is %d\n", *(Tinfo->tid), Tinfo->rwlock,stat);
>              sleep(1);
>          }
>      }while(KM_OK != stat);
>      return NULL;
> }
>
> void main()
> {
>      km_status stat = KM_OK;
>      /* km_rwlock lock; */
>      pthread_rwlock_t rwlock;
>      pthread_t lockerTid = 0, releaseTid = 0, thirdTid=0;
>      struct threadInfo firstTinfo, secondTinfo, thirdTinfo;
>
>     /* stat = km_rwlock_init(&rwlock, KM_TRUE); */
>      stat = pthread_rwlock_init (&rwlock, NULL);
>
>      if (KM_OK == stat) {
>          printf("read\\writer lock initialized\n");
>          printf("Before Thread\n");
>
>          firstTinfo.tid  = &lockerTid;
>          firstTinfo.rwlock = &rwlock;
>          secondTinfo.tid  = &releaseTid;
>          secondTinfo.rwlock = &rwlock;
>          thirdTinfo.tid  = &thirdTid;
>          thirdTinfo.rwlock = &rwlock;
>
>
>          pthread_create(&lockerTid, NULL, lockerThreadFunction, &firstTinfo);
>          pthread_create(&releaseTid,  NULL, secondThreadFunction, &secondTinfo);
>          pthread_create(&thirdTid, NULL, thirdThreadFunction, &thirdTinfo);
>          printf("After Thread %x\n", lockerTid);
>
> pthread_join(lockerTid, NULL);
>          pthread_join(releaseTid, NULL);
>          pthread_join(thirdTid, NULL);
>
>
>
> the output we saw is this:
>
>
> the ouput we saw is this:
>
> [root@Rocky85Mozes thread_lock]# ./rw_test
> read\writer lock initialized
> Before Thread
> [INFO] - FIRST thread 0x461f700 locked 0x7ffcda053cc0
> After Thread 461f700
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16  -> first thread take the rwlock
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [INFO] SECOND thread 0x3e1e700 released 0x7ffcda053cc0   -> Second thread unlock the rwlock -- > and get OK!!
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
> [INFO] FIRST thread 0x461f700 released 0x7ffcda053cc0
> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>
>
> -----Original Message-----
> From: David Mozes
> Sent: Sunday, March 13, 2022 12:23 PM
> To: Florian Weimer <fweimer@redhat.com>
> Cc: libc-help@sourceware.org
> Subject: RE: rwlock different behavior on glibc 2.28 then on previous versions
>
> Thx Florin,
>
>
> -----Original Message-----
> From: Florian Weimer <fweimer@redhat.com>
> Sent: Friday, March 11, 2022 3:59 PM
> To: David Mozes <david.mozes@silk.us>
> Cc: libc-help@sourceware.org
> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
>
> * David Mozes:
>
>> On the previous version of Glibc (before the commit) We could release
>> write lock with different thread as the one that owned the lock (If we
>> like) .
> Sorry, this misuse of the API has always been undefined according to
> POSIX (“Results are undefined if the read-write lock rwlock is not held
> by the calling thread.”).
>
>   >> Correct, but practically we can do it on the user's responsibility.
>>> Do you know if this "if" solved some synchronization issue or just enforced the undefined  API?
>> This is help very much for async program that has a many threads that
>> communicate with some target.  And we like to define a call back
>> function instead of blocking on aio thread that send the data in order
>> to release the rwlock.this improves the performance dramatically .
> Could you use a different synchronization object (perhaps a condition
> variable)?
>
>>> If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states.
>>> If we like to keep the API agreement, why not define the different async programming unlock functions?
> Thanks
> Florian
>
> Thx
> David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-21 14:01         ` David Mozes
@ 2022-03-21 17:01           ` Adhemerval Zanella
  2022-03-21 19:46             ` David Mozes
  0 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2022-03-21 17:01 UTC (permalink / raw)
  To: David Mozes, Christian Hoff, Florian Weimer
  Cc: libc-help, Carlos O'Donell

As Florian has put, this is essentially a misuse of the API has always been 
undefined.  The rwlock implementation uses  __cur_write to both make a fast
deadlock check and to know whether to handle the lock as a writer or reader
one.  The former is a implementation detail that we might change (although
it is useful in some case), but to handle the former will most likely to
refactor and change the rdlock algorithm (just removing the check as you are
proposing is not suffice).

Since it is not a bug in our implementation, I think you will need to discuss
it further on the libc-alpha to see if other are willing to actually make
this change. I foresee that the algorithm change won't be simple to keep
interface requirements and allow the specific case you are asking. In fact
I think it might not be actually possible without change the rwlock semantic
(not sure though).

Carlos has fixed some issue on the rwlock, he might have some comments.

On 21/03/2022 11:01, David Mozes wrote:
> Hello Christain,
> Yes, Thread Sanitizer caught it. 
> However, we can't run it all the time on the production code.
> We need to make a small change of the unlock code to have async programing and backward compatibility for older code.
> 
> Thx
> David  
> 	
> -----Original Message-----
> From: Christian Hoff <christian_hoff@gmx.net> 
> Sent: Monday, March 21, 2022 8:20 AM
> To: David Mozes <david.mozes@silk.us>; Florian Weimer <fweimer@redhat.com>
> Cc: libc-help@sourceware.org
> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
> 
> Hallo David,
> 
> did you try to run your program with Helgrind already? Helgrind was
> designed to catch problems like this. Maybe Thread Sanitizer would also
> detect the issue.
> 
> 
> Best regards,
> 
>     Christian
> 
> On 3/20/22 5:44 PM, David Mozes wrote:
>> Hi,
>> Where are we with this?
>> Also, the current implementation of the __pthread_rwlock_unlock  is problematic.
>> On the one hand, it checks the API agreement. However, on the case of violation, the user don't have any induction
>> And function return OK
>> See example code below:
>>
>>
>> struct threadInfo{
>>      pthread_t* tid;
>>      pthread_rwlock_t *rwlock;
>> };
>>
>> void *lockerThreadFunction(void *vargp)
>> {
>>      km_status stat = KM_OK;
>>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>
>>      do {
>>          stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>>          if (KM_OK == stat) {
>>              printf("[INFO] - FIRST thread 0x%x locked %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>              sleep(10);
>>              stat = pthread_rwlock_unlock(Tinfo->rwlock);
>>              if (KM_OK == stat) {
>>                  printf("[INFO] FIRST thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>                  sleep(1);
>>              }
>>          }
>>      }while(KM_OK != stat);
>>      return NULL;
>> }
>>
>>
>> void *secondThreadFunction(void *vargp)
>> {
>>      km_status stat = KM_OK;
>>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>
>>      sleep(5);
>>      do {
>>          stat = pthread_rwlock_unlock(Tinfo->rwlock);
>>          if (KM_OK == stat) {
>>              printf("[INFO] SECOND thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>              sleep(1);
>>          } else
>>              printf("[INFO] SECOND thread 0x%x try_to_released %p erro status is: %d \n", *(Tinfo->tid), Tinfo->rwlock,stat);
>>
>>      }while(KM_OK != stat);
>>      return NULL;
>> }
>>
>> void *thirdThreadFunction(void *vargp)
>> {
>>      km_status stat = KM_OK;
>>      struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>
>>      sleep(2);
>>      do {
>>          stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>>          if (KM_OK == stat) {
>>              printf("[INFO] THIRD thread 0X%x locked %x\n", *(Tinfo->tid), Tinfo->rwlock);
>>              sleep(5);
>>          }
>>          else {
>>              printf("[ERROR]: THIRD thread 0x%x failed Locking %p status is %d\n", *(Tinfo->tid), Tinfo->rwlock,stat);
>>              sleep(1);
>>          }
>>      }while(KM_OK != stat);
>>      return NULL;
>> }
>>
>> void main()
>> {
>>      km_status stat = KM_OK;
>>      /* km_rwlock lock; */
>>      pthread_rwlock_t rwlock;
>>      pthread_t lockerTid = 0, releaseTid = 0, thirdTid=0;
>>      struct threadInfo firstTinfo, secondTinfo, thirdTinfo;
>>
>>     /* stat = km_rwlock_init(&rwlock, KM_TRUE); */
>>      stat = pthread_rwlock_init (&rwlock, NULL);
>>
>>      if (KM_OK == stat) {
>>          printf("read\\writer lock initialized\n");
>>          printf("Before Thread\n");
>>
>>          firstTinfo.tid  = &lockerTid;
>>          firstTinfo.rwlock = &rwlock;
>>          secondTinfo.tid  = &releaseTid;
>>          secondTinfo.rwlock = &rwlock;
>>          thirdTinfo.tid  = &thirdTid;
>>          thirdTinfo.rwlock = &rwlock;
>>
>>
>>          pthread_create(&lockerTid, NULL, lockerThreadFunction, &firstTinfo);
>>          pthread_create(&releaseTid,  NULL, secondThreadFunction, &secondTinfo);
>>          pthread_create(&thirdTid, NULL, thirdThreadFunction, &thirdTinfo);
>>          printf("After Thread %x\n", lockerTid);
>>
>> pthread_join(lockerTid, NULL);
>>          pthread_join(releaseTid, NULL);
>>          pthread_join(thirdTid, NULL);
>>
>>
>>
>> the output we saw is this:
>>
>>
>> the ouput we saw is this:
>>
>> [root@Rocky85Mozes thread_lock]# ./rw_test
>> read\writer lock initialized
>> Before Thread
>> [INFO] - FIRST thread 0x461f700 locked 0x7ffcda053cc0
>> After Thread 461f700
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16  -> first thread take the rwlock
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>> [INFO] SECOND thread 0x3e1e700 released 0x7ffcda053cc0   -> Second thread unlock the rwlock -- > and get OK!!
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>> [INFO] FIRST thread 0x461f700 released 0x7ffcda053cc0
>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>
>>
>> -----Original Message-----
>> From: David Mozes
>> Sent: Sunday, March 13, 2022 12:23 PM
>> To: Florian Weimer <fweimer@redhat.com>
>> Cc: libc-help@sourceware.org
>> Subject: RE: rwlock different behavior on glibc 2.28 then on previous versions
>>
>> Thx Florin,
>>
>>
>> -----Original Message-----
>> From: Florian Weimer <fweimer@redhat.com>
>> Sent: Friday, March 11, 2022 3:59 PM
>> To: David Mozes <david.mozes@silk.us>
>> Cc: libc-help@sourceware.org
>> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
>>
>> * David Mozes:
>>
>>> On the previous version of Glibc (before the commit) We could release
>>> write lock with different thread as the one that owned the lock (If we
>>> like) .
>> Sorry, this misuse of the API has always been undefined according to
>> POSIX (“Results are undefined if the read-write lock rwlock is not held
>> by the calling thread.”).
>>
>>   >> Correct, but practically we can do it on the user's responsibility.
>>>> Do you know if this "if" solved some synchronization issue or just enforced the undefined  API?
>>> This is help very much for async program that has a many threads that
>>> communicate with some target.  And we like to define a call back
>>> function instead of blocking on aio thread that send the data in order
>>> to release the rwlock.this improves the performance dramatically .
>> Could you use a different synchronization object (perhaps a condition
>> variable)?
>>
>>>> If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states.
>>>> If we like to keep the API agreement, why not define the different async programming unlock functions?
>> Thanks
>> Florian
>>
>> Thx
>> David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-21 17:01           ` Adhemerval Zanella
@ 2022-03-21 19:46             ` David Mozes
  2022-03-21 20:19               ` Adhemerval Zanella
  0 siblings, 1 reply; 10+ messages in thread
From: David Mozes @ 2022-03-21 19:46 UTC (permalink / raw)
  To: Adhemerval Zanella
  Cc: Christian Hoff, Florian Weimer, libc-help, Carlos O'Donell


Thanks Adhemerval,
For the suggestion!

 I suggest to remove the check just on the unlock function.I tested it with very high load and with application with 3k active threads.
 Can you on a case or bug(dedlock or something) that removing thIs check will expose?
Thx
David
On Mar 21, 2022, at 7:02 PM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

As Florian has put, this is essentially a misuse of the API has always been
undefined.  The rwlock implementation uses  __cur_write to both make a fast
deadlock check and to know whether to handle the lock as a writer or reader
one.  The former is a implementation detail that we might change (although
it is useful in some case), but to handle the former will most likely to
refactor and change the rdlock algorithm (just removing the check as you are
proposing is not suffice).

Since it is not a bug in our implementation, I think you will need to discuss
it further on the libc-alpha to see if other are willing to actually make
this change. I foresee that the algorithm change won't be simple to keep
interface requirements and allow the specific case you are asking. In fact
I think it might not be actually possible without change the rwlock semantic
(not sure though).

Carlos has fixed some issue on the rwlock, he might have some comments.

On 21/03/2022 11:01, David Mozes wrote:
Hello Christain,
Yes, Thread Sanitizer caught it.
However, we can't run it all the time on the production code.
We need to make a small change of the unlock code to have async programing and backward compatibility for older code.

Thx
David

-----Original Message-----
From: Christian Hoff <christian_hoff@gmx.net>
Sent: Monday, March 21, 2022 8:20 AM
To: David Mozes <david.mozes@silk.us>; Florian Weimer <fweimer@redhat.com>
Cc: libc-help@sourceware.org
Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions

Hallo David,

did you try to run your program with Helgrind already? Helgrind was
designed to catch problems like this. Maybe Thread Sanitizer would also
detect the issue.


Best regards,

   Christian

On 3/20/22 5:44 PM, David Mozes wrote:
Hi,
Where are we with this?
Also, the current implementation of the __pthread_rwlock_unlock  is problematic.
On the one hand, it checks the API agreement. However, on the case of violation, the user don't have any induction
And function return OK
See example code below:


struct threadInfo{
    pthread_t* tid;
    pthread_rwlock_t *rwlock;
};

void *lockerThreadFunction(void *vargp)
{
    km_status stat = KM_OK;
    struct threadInfo* Tinfo = (struct threadInfo*)vargp;

    do {
        stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
        if (KM_OK == stat) {
            printf("[INFO] - FIRST thread 0x%x locked %p \n", *(Tinfo->tid), Tinfo->rwlock);
            sleep(10);
            stat = pthread_rwlock_unlock(Tinfo->rwlock);
            if (KM_OK == stat) {
                printf("[INFO] FIRST thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
                sleep(1);
            }
        }
    }while(KM_OK != stat);
    return NULL;
}


void *secondThreadFunction(void *vargp)
{
    km_status stat = KM_OK;
    struct threadInfo* Tinfo = (struct threadInfo*)vargp;

    sleep(5);
    do {
        stat = pthread_rwlock_unlock(Tinfo->rwlock);
        if (KM_OK == stat) {
            printf("[INFO] SECOND thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
            sleep(1);
        } else
            printf("[INFO] SECOND thread 0x%x try_to_released %p erro status is: %d \n", *(Tinfo->tid), Tinfo->rwlock,stat);

    }while(KM_OK != stat);
    return NULL;
}

void *thirdThreadFunction(void *vargp)
{
    km_status stat = KM_OK;
    struct threadInfo* Tinfo = (struct threadInfo*)vargp;

    sleep(2);
    do {
        stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
        if (KM_OK == stat) {
            printf("[INFO] THIRD thread 0X%x locked %x\n", *(Tinfo->tid), Tinfo->rwlock);
            sleep(5);
        }
        else {
            printf("[ERROR]: THIRD thread 0x%x failed Locking %p status is %d\n", *(Tinfo->tid), Tinfo->rwlock,stat);
            sleep(1);
        }
    }while(KM_OK != stat);
    return NULL;
}

void main()
{
    km_status stat = KM_OK;
    /* km_rwlock lock; */
    pthread_rwlock_t rwlock;
    pthread_t lockerTid = 0, releaseTid = 0, thirdTid=0;
    struct threadInfo firstTinfo, secondTinfo, thirdTinfo;

   /* stat = km_rwlock_init(&rwlock, KM_TRUE); */
    stat = pthread_rwlock_init (&rwlock, NULL);

    if (KM_OK == stat) {
        printf("read\\writer lock initialized\n");
        printf("Before Thread\n");

        firstTinfo.tid  = &lockerTid;
        firstTinfo.rwlock = &rwlock;
        secondTinfo.tid  = &releaseTid;
        secondTinfo.rwlock = &rwlock;
        thirdTinfo.tid  = &thirdTid;
        thirdTinfo.rwlock = &rwlock;


        pthread_create(&lockerTid, NULL, lockerThreadFunction, &firstTinfo);
        pthread_create(&releaseTid,  NULL, secondThreadFunction, &secondTinfo);
        pthread_create(&thirdTid, NULL, thirdThreadFunction, &thirdTinfo);
        printf("After Thread %x\n", lockerTid);

pthread_join(lockerTid, NULL);
        pthread_join(releaseTid, NULL);
        pthread_join(thirdTid, NULL);



the output we saw is this:


the ouput we saw is this:

[root@Rocky85Mozes thread_lock]# ./rw_test
read\writer lock initialized
Before Thread
[INFO] - FIRST thread 0x461f700 locked 0x7ffcda053cc0
After Thread 461f700
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16  -> first thread take the rwlock
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[INFO] SECOND thread 0x3e1e700 released 0x7ffcda053cc0   -> Second thread unlock the rwlock -- > and get OK!!
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
[INFO] FIRST thread 0x461f700 released 0x7ffcda053cc0
[ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16


-----Original Message-----
From: David Mozes
Sent: Sunday, March 13, 2022 12:23 PM
To: Florian Weimer <fweimer@redhat.com>
Cc: libc-help@sourceware.org
Subject: RE: rwlock different behavior on glibc 2.28 then on previous versions

Thx Florin,


-----Original Message-----
From: Florian Weimer <fweimer@redhat.com>
Sent: Friday, March 11, 2022 3:59 PM
To: David Mozes <david.mozes@silk.us>
Cc: libc-help@sourceware.org
Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions

* David Mozes:

On the previous version of Glibc (before the commit) We could release
write lock with different thread as the one that owned the lock (If we
like) .
Sorry, this misuse of the API has always been undefined according to
POSIX (“Results are undefined if the read-write lock rwlock is not held
by the calling thread.”).

Correct, but practically we can do it on the user's responsibility.
Do you know if this "if" solved some synchronization issue or just enforced the undefined  API?
This is help very much for async program that has a many threads that
communicate with some target.  And we like to define a call back
function instead of blocking on aio thread that send the data in order
to release the rwlock.this improves the performance dramatically .
Could you use a different synchronization object (perhaps a condition
variable)?

If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states.
If we like to keep the API agreement, why not define the different async programming unlock functions?
Thanks
Florian

Thx
David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-21 19:46             ` David Mozes
@ 2022-03-21 20:19               ` Adhemerval Zanella
  2022-03-24 15:41                 ` David Mozes
  0 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2022-03-21 20:19 UTC (permalink / raw)
  To: David Mozes
  Cc: Christian Hoff, Florian Weimer, libc-help, Carlos O'Donell

On 21/03/2022 16:46, David Mozes wrote:
> 
> Thanks Adhemerval,
> For the suggestion!
>  
>  I suggest to remove the check just on the unlock function.I tested it with very high load and with application with 3k active threads. 
>  Can you on a case or bug(dedlock or something) that removing thIs check will expose?

I am no sure, so I would suggest you to maybe start a discussion on libc-alpha.
However __pthread_rwlock_wrunlock and __pthread_rwlock_rdunlock have different
semantic allowing unlocking write locks on different threads will require a
lot of analysis on why it would trigger any more regression (and sorry, but 
stating it works on our environment is not suffice).

> Thx
> David
>> On Mar 21, 2022, at 7:02 PM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>
>> As Florian has put, this is essentially a misuse of the API has always been
>> undefined.  The rwlock implementation uses  __cur_write to both make a fast
>> deadlock check and to know whether to handle the lock as a writer or reader
>> one.  The former is a implementation detail that we might change (although
>> it is useful in some case), but to handle the former will most likely to
>> refactor and change the rdlock algorithm (just removing the check as you are
>> proposing is not suffice).
>>
>> Since it is not a bug in our implementation, I think you will need to discuss
>> it further on the libc-alpha to see if other are willing to actually make
>> this change. I foresee that the algorithm change won't be simple to keep
>> interface requirements and allow the specific case you are asking. In fact
>> I think it might not be actually possible without change the rwlock semantic
>> (not sure though).
>>
>> Carlos has fixed some issue on the rwlock, he might have some comments.
>>
>> On 21/03/2022 11:01, David Mozes wrote:
>>> Hello Christain,
>>> Yes, Thread Sanitizer caught it.
>>> However, we can't run it all the time on the production code.
>>> We need to make a small change of the unlock code to have async programing and backward compatibility for older code.
>>>
>>> Thx
>>> David  
>>>    
>>> -----Original Message-----
>>> From: Christian Hoff <christian_hoff@gmx.net>
>>> Sent: Monday, March 21, 2022 8:20 AM
>>> To: David Mozes <david.mozes@silk.us>; Florian Weimer <fweimer@redhat.com>
>>> Cc: libc-help@sourceware.org
>>> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
>>>
>>> Hallo David,
>>>
>>> did you try to run your program with Helgrind already? Helgrind was
>>> designed to catch problems like this. Maybe Thread Sanitizer would also
>>> detect the issue.
>>>
>>>
>>> Best regards,
>>>
>>>    Christian
>>>
>>> On 3/20/22 5:44 PM, David Mozes wrote:
>>>> Hi,
>>>> Where are we with this?
>>>> Also, the current implementation of the __pthread_rwlock_unlock  is problematic.
>>>> On the one hand, it checks the API agreement. However, on the case of violation, the user don't have any induction
>>>> And function return OK
>>>> See example code below:
>>>>
>>>>
>>>> struct threadInfo{
>>>>     pthread_t* tid;
>>>>     pthread_rwlock_t *rwlock;
>>>> };
>>>>
>>>> void *lockerThreadFunction(void *vargp)
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>>>
>>>>     do {
>>>>         stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>>>>         if (KM_OK == stat) {
>>>>             printf("[INFO] - FIRST thread 0x%x locked %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>>>             sleep(10);
>>>>             stat = pthread_rwlock_unlock(Tinfo->rwlock);
>>>>             if (KM_OK == stat) {
>>>>                 printf("[INFO] FIRST thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>>>                 sleep(1);
>>>>             }
>>>>         }
>>>>     }while(KM_OK != stat);
>>>>     return NULL;
>>>> }
>>>>
>>>>
>>>> void *secondThreadFunction(void *vargp)
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>>>
>>>>     sleep(5);
>>>>     do {
>>>>         stat = pthread_rwlock_unlock(Tinfo->rwlock);
>>>>         if (KM_OK == stat) {
>>>>             printf("[INFO] SECOND thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>>>             sleep(1);
>>>>         } else
>>>>             printf("[INFO] SECOND thread 0x%x try_to_released %p erro status is: %d \n", *(Tinfo->tid), Tinfo->rwlock,stat);
>>>>
>>>>     }while(KM_OK != stat);
>>>>     return NULL;
>>>> }
>>>>
>>>> void *thirdThreadFunction(void *vargp)
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>>>
>>>>     sleep(2);
>>>>     do {
>>>>         stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>>>>         if (KM_OK == stat) {
>>>>             printf("[INFO] THIRD thread 0X%x locked %x\n", *(Tinfo->tid), Tinfo->rwlock);
>>>>             sleep(5);
>>>>         }
>>>>         else {
>>>>             printf("[ERROR]: THIRD thread 0x%x failed Locking %p status is %d\n", *(Tinfo->tid), Tinfo->rwlock,stat);
>>>>             sleep(1);
>>>>         }
>>>>     }while(KM_OK != stat);
>>>>     return NULL;
>>>> }
>>>>
>>>> void main()
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     /* km_rwlock lock; */
>>>>     pthread_rwlock_t rwlock;
>>>>     pthread_t lockerTid = 0, releaseTid = 0, thirdTid=0;
>>>>     struct threadInfo firstTinfo, secondTinfo, thirdTinfo;
>>>>
>>>>    /* stat = km_rwlock_init(&rwlock, KM_TRUE); */
>>>>     stat = pthread_rwlock_init (&rwlock, NULL);
>>>>
>>>>     if (KM_OK == stat) {
>>>>         printf("read\\writer lock initialized\n");
>>>>         printf("Before Thread\n");
>>>>
>>>>         firstTinfo.tid  = &lockerTid;
>>>>         firstTinfo.rwlock = &rwlock;
>>>>         secondTinfo.tid  = &releaseTid;
>>>>         secondTinfo.rwlock = &rwlock;
>>>>         thirdTinfo.tid  = &thirdTid;
>>>>         thirdTinfo.rwlock = &rwlock;
>>>>
>>>>
>>>>         pthread_create(&lockerTid, NULL, lockerThreadFunction, &firstTinfo);
>>>>         pthread_create(&releaseTid,  NULL, secondThreadFunction, &secondTinfo);
>>>>         pthread_create(&thirdTid, NULL, thirdThreadFunction, &thirdTinfo);
>>>>         printf("After Thread %x\n", lockerTid);
>>>>
>>>> pthread_join(lockerTid, NULL);
>>>>         pthread_join(releaseTid, NULL);
>>>>         pthread_join(thirdTid, NULL);
>>>>
>>>>
>>>>
>>>> the output we saw is this:
>>>>
>>>>
>>>> the ouput we saw is this:
>>>>
>>>> [root@Rocky85Mozes thread_lock]# ./rw_test
>>>> read\writer lock initialized
>>>> Before Thread
>>>> [INFO] - FIRST thread 0x461f700 locked 0x7ffcda053cc0
>>>> After Thread 461f700
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16  -> first thread take the rwlock
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [INFO] SECOND thread 0x3e1e700 released 0x7ffcda053cc0   -> Second thread unlock the rwlock -- > and get OK!!
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [INFO] FIRST thread 0x461f700 released 0x7ffcda053cc0
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: David Mozes
>>>> Sent: Sunday, March 13, 2022 12:23 PM
>>>> To: Florian Weimer <fweimer@redhat.com>
>>>> Cc: libc-help@sourceware.org
>>>> Subject: RE: rwlock different behavior on glibc 2.28 then on previous versions
>>>>
>>>> Thx Florin,
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Florian Weimer <fweimer@redhat.com>
>>>> Sent: Friday, March 11, 2022 3:59 PM
>>>> To: David Mozes <david.mozes@silk.us>
>>>> Cc: libc-help@sourceware.org
>>>> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
>>>>
>>>> * David Mozes:
>>>>
>>>>> On the previous version of Glibc (before the commit) We could release
>>>>> write lock with different thread as the one that owned the lock (If we
>>>>> like) .
>>>> Sorry, this misuse of the API has always been undefined according to
>>>> POSIX (“Results are undefined if the read-write lock rwlock is not held
>>>> by the calling thread.”).
>>>>
>>>>>> Correct, but practically we can do it on the user's responsibility.
>>>>>> Do you know if this "if" solved some synchronization issue or just enforced the undefined  API?
>>>>> This is help very much for async program that has a many threads that
>>>>> communicate with some target.  And we like to define a call back
>>>>> function instead of blocking on aio thread that send the data in order
>>>>> to release the rwlock.this improves the performance dramatically .
>>>> Could you use a different synchronization object (perhaps a condition
>>>> variable)?
>>>>
>>>>>> If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states.
>>>>>> If we like to keep the API agreement, why not define the different async programming unlock functions?
>>>> Thanks
>>>> Florian
>>>>
>>>> Thx
>>>> David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: rwlock different behavior on glibc 2.28 then on previous versions
  2022-03-21 20:19               ` Adhemerval Zanella
@ 2022-03-24 15:41                 ` David Mozes
  0 siblings, 0 replies; 10+ messages in thread
From: David Mozes @ 2022-03-24 15:41 UTC (permalink / raw)
  To: Adhemerval Zanella
  Cc: Christian Hoff, Florian Weimer, libc-help, Carlos O'Donell

Thx sure!
I will start a discussion on the Alpha soon.
So you mean that  if (atomic_load_relaxed (&rwlock->__data.__cur_writer)
-      == THREAD_GETMEM (THREAD_SELF, tid))
Used the THREAD_GETMEM (THREAD_SELF, tid)) for protection regarding the load_releax ?

You may have a suggestion on how to make async programming with rw_lock? 

Thx
David 

-----Original Message-----
From: Adhemerval Zanella <adhemerval.zanella@linaro.org> 
Sent: Monday, March 21, 2022 10:20 PM
To: David Mozes <david.mozes@silk.us>
Cc: Christian Hoff <christian_hoff@gmx.net>; Florian Weimer <fweimer@redhat.com>; libc-help@sourceware.org; Carlos O'Donell <carlos@redhat.com>
Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions

On 21/03/2022 16:46, David Mozes wrote:
> 
> Thanks Adhemerval,
> For the suggestion!
>  
>  I suggest to remove the check just on the unlock function.I tested it with very high load and with application with 3k active threads. 
>  Can you on a case or bug(dedlock or something) that removing thIs check will expose?

I am no sure, so I would suggest you to maybe start a discussion on libc-alpha.
However __pthread_rwlock_wrunlock and __pthread_rwlock_rdunlock have different
semantic allowing unlocking write locks on different threads will require a
lot of analysis on why it would trigger any more regression (and sorry, but 
stating it works on our environment is not suffice).

> Thx
> David
>> On Mar 21, 2022, at 7:02 PM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>
>> As Florian has put, this is essentially a misuse of the API has always been
>> undefined.  The rwlock implementation uses  __cur_write to both make a fast
>> deadlock check and to know whether to handle the lock as a writer or reader
>> one.  The former is a implementation detail that we might change (although
>> it is useful in some case), but to handle the former will most likely to
>> refactor and change the rdlock algorithm (just removing the check as you are
>> proposing is not suffice).
>>
>> Since it is not a bug in our implementation, I think you will need to discuss
>> it further on the libc-alpha to see if other are willing to actually make
>> this change. I foresee that the algorithm change won't be simple to keep
>> interface requirements and allow the specific case you are asking. In fact
>> I think it might not be actually possible without change the rwlock semantic
>> (not sure though).
>>
>> Carlos has fixed some issue on the rwlock, he might have some comments.
>>
>> On 21/03/2022 11:01, David Mozes wrote:
>>> Hello Christain,
>>> Yes, Thread Sanitizer caught it.
>>> However, we can't run it all the time on the production code.
>>> We need to make a small change of the unlock code to have async programing and backward compatibility for older code.
>>>
>>> Thx
>>> David  
>>>    
>>> -----Original Message-----
>>> From: Christian Hoff <christian_hoff@gmx.net>
>>> Sent: Monday, March 21, 2022 8:20 AM
>>> To: David Mozes <david.mozes@silk.us>; Florian Weimer <fweimer@redhat.com>
>>> Cc: libc-help@sourceware.org
>>> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
>>>
>>> Hallo David,
>>>
>>> did you try to run your program with Helgrind already? Helgrind was
>>> designed to catch problems like this. Maybe Thread Sanitizer would also
>>> detect the issue.
>>>
>>>
>>> Best regards,
>>>
>>>    Christian
>>>
>>> On 3/20/22 5:44 PM, David Mozes wrote:
>>>> Hi,
>>>> Where are we with this?
>>>> Also, the current implementation of the __pthread_rwlock_unlock  is problematic.
>>>> On the one hand, it checks the API agreement. However, on the case of violation, the user don't have any induction
>>>> And function return OK
>>>> See example code below:
>>>>
>>>>
>>>> struct threadInfo{
>>>>     pthread_t* tid;
>>>>     pthread_rwlock_t *rwlock;
>>>> };
>>>>
>>>> void *lockerThreadFunction(void *vargp)
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>>>
>>>>     do {
>>>>         stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>>>>         if (KM_OK == stat) {
>>>>             printf("[INFO] - FIRST thread 0x%x locked %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>>>             sleep(10);
>>>>             stat = pthread_rwlock_unlock(Tinfo->rwlock);
>>>>             if (KM_OK == stat) {
>>>>                 printf("[INFO] FIRST thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>>>                 sleep(1);
>>>>             }
>>>>         }
>>>>     }while(KM_OK != stat);
>>>>     return NULL;
>>>> }
>>>>
>>>>
>>>> void *secondThreadFunction(void *vargp)
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>>>
>>>>     sleep(5);
>>>>     do {
>>>>         stat = pthread_rwlock_unlock(Tinfo->rwlock);
>>>>         if (KM_OK == stat) {
>>>>             printf("[INFO] SECOND thread 0x%x released %p \n", *(Tinfo->tid), Tinfo->rwlock);
>>>>             sleep(1);
>>>>         } else
>>>>             printf("[INFO] SECOND thread 0x%x try_to_released %p erro status is: %d \n", *(Tinfo->tid), Tinfo->rwlock,stat);
>>>>
>>>>     }while(KM_OK != stat);
>>>>     return NULL;
>>>> }
>>>>
>>>> void *thirdThreadFunction(void *vargp)
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     struct threadInfo* Tinfo = (struct threadInfo*)vargp;
>>>>
>>>>     sleep(2);
>>>>     do {
>>>>         stat = pthread_rwlock_trywrlock(Tinfo->rwlock);
>>>>         if (KM_OK == stat) {
>>>>             printf("[INFO] THIRD thread 0X%x locked %x\n", *(Tinfo->tid), Tinfo->rwlock);
>>>>             sleep(5);
>>>>         }
>>>>         else {
>>>>             printf("[ERROR]: THIRD thread 0x%x failed Locking %p status is %d\n", *(Tinfo->tid), Tinfo->rwlock,stat);
>>>>             sleep(1);
>>>>         }
>>>>     }while(KM_OK != stat);
>>>>     return NULL;
>>>> }
>>>>
>>>> void main()
>>>> {
>>>>     km_status stat = KM_OK;
>>>>     /* km_rwlock lock; */
>>>>     pthread_rwlock_t rwlock;
>>>>     pthread_t lockerTid = 0, releaseTid = 0, thirdTid=0;
>>>>     struct threadInfo firstTinfo, secondTinfo, thirdTinfo;
>>>>
>>>>    /* stat = km_rwlock_init(&rwlock, KM_TRUE); */
>>>>     stat = pthread_rwlock_init (&rwlock, NULL);
>>>>
>>>>     if (KM_OK == stat) {
>>>>         printf("read\\writer lock initialized\n");
>>>>         printf("Before Thread\n");
>>>>
>>>>         firstTinfo.tid  = &lockerTid;
>>>>         firstTinfo.rwlock = &rwlock;
>>>>         secondTinfo.tid  = &releaseTid;
>>>>         secondTinfo.rwlock = &rwlock;
>>>>         thirdTinfo.tid  = &thirdTid;
>>>>         thirdTinfo.rwlock = &rwlock;
>>>>
>>>>
>>>>         pthread_create(&lockerTid, NULL, lockerThreadFunction, &firstTinfo);
>>>>         pthread_create(&releaseTid,  NULL, secondThreadFunction, &secondTinfo);
>>>>         pthread_create(&thirdTid, NULL, thirdThreadFunction, &thirdTinfo);
>>>>         printf("After Thread %x\n", lockerTid);
>>>>
>>>> pthread_join(lockerTid, NULL);
>>>>         pthread_join(releaseTid, NULL);
>>>>         pthread_join(thirdTid, NULL);
>>>>
>>>>
>>>>
>>>> the output we saw is this:
>>>>
>>>>
>>>> the ouput we saw is this:
>>>>
>>>> [root@Rocky85Mozes thread_lock]# ./rw_test
>>>> read\writer lock initialized
>>>> Before Thread
>>>> [INFO] - FIRST thread 0x461f700 locked 0x7ffcda053cc0
>>>> After Thread 461f700
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16  -> first thread take the rwlock
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [INFO] SECOND thread 0x3e1e700 released 0x7ffcda053cc0   -> Second thread unlock the rwlock -- > and get OK!!
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>> [INFO] FIRST thread 0x461f700 released 0x7ffcda053cc0
>>>> [ERROR]: THIRD thread 0x361d700 failed Locking 0x7ffcda053cc0 status is 16
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: David Mozes
>>>> Sent: Sunday, March 13, 2022 12:23 PM
>>>> To: Florian Weimer <fweimer@redhat.com>
>>>> Cc: libc-help@sourceware.org
>>>> Subject: RE: rwlock different behavior on glibc 2.28 then on previous versions
>>>>
>>>> Thx Florin,
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Florian Weimer <fweimer@redhat.com>
>>>> Sent: Friday, March 11, 2022 3:59 PM
>>>> To: David Mozes <david.mozes@silk.us>
>>>> Cc: libc-help@sourceware.org
>>>> Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
>>>>
>>>> * David Mozes:
>>>>
>>>>> On the previous version of Glibc (before the commit) We could release
>>>>> write lock with different thread as the one that owned the lock (If we
>>>>> like) .
>>>> Sorry, this misuse of the API has always been undefined according to
>>>> POSIX (“Results are undefined if the read-write lock rwlock is not held
>>>> by the calling thread.”).
>>>>
>>>>>> Correct, but practically we can do it on the user's responsibility.
>>>>>> Do you know if this "if" solved some synchronization issue or just enforced the undefined  API?
>>>>> This is help very much for async program that has a many threads that
>>>>> communicate with some target.  And we like to define a call back
>>>>> function instead of blocking on aio thread that send the data in order
>>>>> to release the rwlock.this improves the performance dramatically .
>>>> Could you use a different synchronization object (perhaps a condition
>>>> variable)?
>>>>
>>>>>> If we use a different synchronization object like semaphores, condition variable (by the way they use mutex under the same API agreement),  we will need to write self rwlock , which has less performance than other undefined states.
>>>>>> If we like to keep the API agreement, why not define the different async programming unlock functions?
>>>> Thanks
>>>> Florian
>>>>
>>>> Thx
>>>> David

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-03-24 15:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 20:13 rwlock different behavior on glibc 2.28 then on previous versions David Mozes
2022-03-11 13:58 ` Florian Weimer
2022-03-13 10:23   ` David Mozes
2022-03-20 16:44     ` David Mozes
2022-03-21  6:19       ` Christian Hoff
2022-03-21 14:01         ` David Mozes
2022-03-21 17:01           ` Adhemerval Zanella
2022-03-21 19:46             ` David Mozes
2022-03-21 20:19               ` Adhemerval Zanella
2022-03-24 15:41                 ` David Mozes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).