[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.