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

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

On 13.03.20 12:39, Julien Grall wrote:
> Hi Juergen,
>
> On 13/03/2020 11:23, Jürgen Groß wrote:
> > 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.
> I am aware of that...
>
> > Even with preemption added the probability to be preempted in a sequence
> > of about a dozen instructions is _very_ low.
> ... but I am not convinced of the low probability here.
>
> > > > 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.
> Do you realize that the lock is most likely be uncontented? And if it 
> were, the caller would likely not spin in a tight loop, otherwise it 
> would have used read_lock().
> So until you proved me otherwise (with numbers), this is 
> micro-optimization that is not going to be seen in a workload.
Fine, in case you feeling so strong about that, I'll change it.


Juergen

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