Re: [Xen-devel] [PATCH 1/2] xen/rwlocks: call preempt_disable() when taking a rwlock

[Jürgen Groß <jgross@xxxxxxxx>]

On 13.03.20 11:40, Julien Grall wrote:
> Hi Juergen,
>
> On 13/03/2020 10:15, Jürgen Groß wrote:
> > On 13.03.20 11:02, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 13/03/2020 08:05, Juergen Gross wrote:
> > > > Similar to spinlocks preemption should be disabled while holding a
> > > > rwlock.
> > > >
> > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > ---
> > > >   xen/include/xen/rwlock.h | 18 +++++++++++++++++-
> > > >   1 file changed, 17 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/include/xen/rwlock.h b/xen/include/xen/rwlock.h
> > > > index 1c221dd0d9..4ee341a182 100644
> > > > --- a/xen/include/xen/rwlock.h
> > > > +++ b/xen/include/xen/rwlock.h
> > > > @@ -2,6 +2,7 @@
> > > >   #define __RWLOCK_H__
> > > >   #include <xen/percpu.h>
> > > > +#include <xen/preempt.h>
> > > >   #include <xen/smp.h>
> > > >   #include <xen/spinlock.h>
> > > > @@ -57,10 +58,12 @@ static inline int _read_trylock(rwlock_t *lock)
> > > >       cnts = atomic_read(&lock->cnts);
> > > >       if ( likely(_can_read_lock(cnts)) )
> > > >       {
> > > If you get preempted here, then it means the check below is likely 
> > > going to fail. So I think it would be best to disable preemption 
> > > before, to give more chance to succeed.
> > As preemption probability at this very point should be much lower than
> > that of held locks I think that is optimizing the wrong path.
> Why so? Lock contention should be fairly limited or you already have a 
> problem on your system. So preemption is more likely.
Today probability of preemption is 0.

Even with preemption added the probability to be preempted in a sequence
of about a dozen instructions is _very_ low.

> > I'm not
> > opposed doing the modification you are requesting, but would like to
> > hear a second opinion on that topic, especially as I'd need to add
> > another preempt_enable() call when following your advice.
> I don't really see the problem with adding a new preemption_enable() 
> call. But the code can also be reworked to have only one call...
Its the dynamical path I'm speaking of. Accessing a local cpu variable
is not that cheap, and in case we add preemption in the future
preempt_enable() will become even more expensive.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel