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

[Julien Grall <julien@xxxxxxx>]

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.
> 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...
Cheers,


--
Julien Grall

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