[PATCH] nptl: pthread_rwlock_rdlock return in low priority

[PATCH] nptl: pthread_rwlock_rdlock return in low priority

[abushwang]

hi, I have noticed reader will return directly on fast-path in pthread_rwlock_common.c

>  /* We have registered as a reader, so if we are in a read phase, we have
>     acquired a read lock.  This is also the reader--reader fast-path.
>     Even if there is a primary writer, we just return.  If writers are to
>     be preferred and we are the only active reader, we could try to enter a
>     write phase to let the writer proceed.  This would be okay because we
>     cannot have acquired the lock previously as a reader (which could result
>     in deadlock if we would wait for the primary writer to run).  However,
>     this seems to be a corner case and handling it specially not be worth the
>     complexity.  */
>  if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0))
>    return 0;

However, there is a situation:
    main, thread_wr, thread_rd.

    SCHED_FIFO priority:
        main > thread_wr > thread_rd
    main first acquires read lock, then create thread_wr which will block on the lock.
    Next, main creates thread_rd. this thread will acquires read lock on fast-path even
    though it has a lower priority compared to thread_wr.

You can get demo from the following repository:
https://github.com/emscripten-core/posixtestsuite.git
./conformance/interfaces/pthread_rwlock_rdlock/2-1.c

According to "man -M man-pages-posix-2017/ 3p pthread_rwlock_rdlock"

> DESCRIPTION
> The pthread_rwlock_rdlock() function shall apply a read lock to the
> read-write lock referenced by rwlock.  The calling thread acquires the
> read lock  if  a writer does not hold the lock and there are no
> writers blocked on the lock.
>
> If  the  Thread  Execution  Scheduling  option  is supported,  and the
> threads involved in the lock are executing with the scheduling
> policies SCHED_FIFO or SCHED_RR, the calling thread shall not acquire
> the lock if a writer holds the lock or if writers of higher or equal
> priority are blocked on the lock;  other‐ wise, the calling thread
> shall acquire the lock.

I was wondering that whether this fast-path is reasonable, and whether
this posix standard should be enforced.

Thanks

Signed-off-by: abushwang <abushwangs@gmail.com>
---
 nptl/pthread_rwlock_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
index 5266a00ed1..35b00fc14f 100644
--- a/nptl/pthread_rwlock_common.c
+++ b/nptl/pthread_rwlock_common.c
@@ -389,6 +389,7 @@ __pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid,
      complexity.  */
   if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0))
     return 0;
+
   /* Otherwise, if we were in a write phase (states #6 or #8), we must wait
      for explicit hand-over of the read phase; the only exception is if we
      can start a read phase if there is no primary writer currently.  */
-- 
2.36.1

Re: [PATCH] nptl: pthread_rwlock_rdlock return in low priority

[Xi Ruoyao]

On Tue, 2023-03-07 at 11:50 +0800, abushwang via Libc-alpha wrote:
> Signed-off-by: abushwang <abushwangs@gmail.com>
> ---
>  nptl/pthread_rwlock_common.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
> index 5266a00ed1..35b00fc14f 100644
> --- a/nptl/pthread_rwlock_common.c
> +++ b/nptl/pthread_rwlock_common.c
> @@ -389,6 +389,7 @@ __pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid,
>       complexity.  */
>    if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0))
>      return 0;
> +

I don't think simply adding an empty line can fix anything.

>    /* Otherwise, if we were in a write phase (states #6 or #8), we must wait
>       for explicit hand-over of the read phase; the only exception is if we
>       can start a read phase if there is no primary writer currently.  */
> --
> 2.36.1

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] nptl: pthread_rwlock_rdlock return in low priority

[abush wang]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

ignore this empty line, i just want send this mail

Xi Ruoyao <xry111@xry111.site> 于2023年3月7日周二 16:40写道：

> On Tue, 2023-03-07 at 11:50 +0800, abushwang via Libc-alpha wrote:
> > Signed-off-by: abushwang <abushwangs@gmail.com>
> > ---
> >  nptl/pthread_rwlock_common.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
> > index 5266a00ed1..35b00fc14f 100644
> > --- a/nptl/pthread_rwlock_common.c
> > +++ b/nptl/pthread_rwlock_common.c
> > @@ -389,6 +389,7 @@ __pthread_rwlock_rdlock_full64 (pthread_rwlock_t
> *rwlock, clockid_t clockid,
> >       complexity.  */
> >    if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0))
> >      return 0;
> > +
>
> I don't think simply adding an empty line can fix anything.
>
> >    /* Otherwise, if we were in a write phase (states #6 or #8), we must
> wait
> >       for explicit hand-over of the read phase; the only exception is if
> we
> >       can start a read phase if there is no primary writer currently.  */
> > --
> > 2.36.1
>
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University
>

Re: [PATCH] nptl: pthread_rwlock_rdlock return in low priority

[Florian Weimer]

* abushwang via Libc-alpha:

> diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
> index 5266a00ed1..35b00fc14f 100644
> --- a/nptl/pthread_rwlock_common.c
> +++ b/nptl/pthread_rwlock_common.c
> @@ -389,6 +389,7 @@ __pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid,
>       complexity.  */
>    if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0))
>      return 0;
> +
>    /* Otherwise, if we were in a write phase (states #6 or #8), we must wait
>       for explicit hand-over of the read phase; the only exception is if we
>       can start a read phase if there is no primary writer currently.  */

This does not seem to be the right patch?

Thanks,
Florian

Re: [PATCH] nptl: pthread_rwlock_rdlock return in low priority

[abush wang]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

Actually, i can not send mail without any change by git sendmail, so I add
a empty line.
This is just a mail for advisory

Florian Weimer <fweimer@redhat.com> 于2023年3月7日周二 16:56写道：

> * abushwang via Libc-alpha:
>
> > diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
> > index 5266a00ed1..35b00fc14f 100644
> > --- a/nptl/pthread_rwlock_common.c
> > +++ b/nptl/pthread_rwlock_common.c
> > @@ -389,6 +389,7 @@ __pthread_rwlock_rdlock_full64 (pthread_rwlock_t
> *rwlock, clockid_t clockid,
> >       complexity.  */
> >    if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0))
> >      return 0;
> > +
> >    /* Otherwise, if we were in a write phase (states #6 or #8), we must
> wait
> >       for explicit hand-over of the read phase; the only exception is if
> we
> >       can start a read phase if there is no primary writer currently.  */
>
> This does not seem to be the right patch?
>
> Thanks,
> Florian
>
>

Re: [PATCH] nptl: pthread_rwlock_rdlock return in low priority

[Xi Ruoyao]

On Tue, 2023-03-07 at 17:06 +0800, abush wang via Libc-alpha wrote:
> Actually, i can not send mail without any change by git sendmail, so I
> add
> a empty line.
> This is just a mail for advisory

Don't use git send-email then.  It's a utility to send patches as mails,
not ordinary mails.

If you need a CLI tool for email use sendmail or something.
> 
> Florian Weimer <fweimer@redhat.com> 于2023年3月7日周二 16:56写道：
> 
> > * abushwang via Libc-alpha:
> > 
> > > diff --git a/nptl/pthread_rwlock_common.c
> > > b/nptl/pthread_rwlock_common.c
> > > index 5266a00ed1..35b00fc14f 100644
> > > --- a/nptl/pthread_rwlock_common.c
> > > +++ b/nptl/pthread_rwlock_common.c
> > > @@ -389,6 +389,7 @@ __pthread_rwlock_rdlock_full64
> > > (pthread_rwlock_t
> > *rwlock, clockid_t clockid,
> > >       complexity.  */
> > >    if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0))
> > >      return 0;
> > > +
> > >    /* Otherwise, if we were in a write phase (states #6 or #8), we
> > > must
> > wait
> > >       for explicit hand-over of the read phase; the only exception
> > > is if
> > we
> > >       can start a read phase if there is no primary writer
> > > currently.  */
> > 
> > This does not seem to be the right patch?
> > 
> > Thanks,
> > Florian
> > 
> > 

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University