public inbox for libc-help@sourceware.org
 help / color / mirror / Atom feed
From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: David Mozes <david.mozes@silk.us>
Cc: Christian Hoff <christian_hoff@gmx.net>,
	Florian Weimer <fweimer@redhat.com>,
	"libc-help@sourceware.org" <libc-help@sourceware.org>,
	Carlos O'Donell <carlos@redhat.com>
Subject: Re: rwlock different behavior on glibc 2.28 then on previous versions
Date: Mon, 21 Mar 2022 17:19:49 -0300	[thread overview]
Message-ID: <0599084c-49bc-23a3-da7e-ccb197d4c88c@linaro.org> (raw)
In-Reply-To: <6D2D3001-2308-4336-829F-8369DF7D2514@silk.us>

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

  reply	other threads:[~2022-03-21 20:19 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-07 20:13 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 [this message]
2022-03-24 15:41                 ` David Mozes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0599084c-49bc-23a3-da7e-ccb197d4c88c@linaro.org \
    --to=adhemerval.zanella@linaro.org \
    --cc=carlos@redhat.com \
    --cc=christian_hoff@gmx.net \
    --cc=david.mozes@silk.us \
    --cc=fweimer@redhat.com \
    --cc=libc-help@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).