[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] xen/rwlock: add check_lock() handling to rwlocks
On 03.11.20 11:04, Jan Beulich wrote:
On 03.11.2020 10:22, Jürgen Groß wrote:
On 03.11.20 10:02, Jan Beulich wrote:
On 02.11.2020 14:12, Juergen Gross wrote:
--- a/xen/include/xen/rwlock.h
+++ b/xen/include/xen/rwlock.h
@@ -56,6 +56,7 @@ static inline int _read_trylock(rwlock_t *lock)
u32 cnts;
preempt_disable();
+ check_lock(&lock->lock.debug, true);
cnts = atomic_read(&lock->cnts);
if ( likely(_can_read_lock(cnts)) )
{
I'm sorry for being picky, but this still isn't matching
_spin_trylock(). Perhaps the difference is really benign, but
there the check sits ahead of preempt_disable(). (It has a
clear reason to be this way there, but without a clear reason
for things to be differently here, I think matching ordering
may help, at least to avoid questions.)
I think this is more an optimization opportunity: I'd rather move the
preempt_disable() into the first if clause, as there is no need to
disable preemption in case the first read of the lock already leads
to failure acquiring the lock.
If you want I can prepend a patch doing that optimization.
I'd appreciate you doing so, yet I'd like to point out that
then the same question remains for _write_trylock(). Perhaps
a similar transformation is possible there, but it'll at
least be more code churn. Which of course isn't a reason not
to go this route there too.
Shouldn't be very hard. It would just need to split the if clause
into two clauses.
This said - wouldn't what you suggest be wrong if we had
actual preemption in the hypervisor? Preemption hitting
between e.g. these two lines
cnts = atomic_read(&lock->cnts);
if ( likely(_can_read_lock(cnts)) )
would not yield the intended result, would it? (It wouldn't
affect correctness afaics, because the caller has to be
prepared anyway that the attempt fails, but the amount of
effectively false negatives would grow, as would the number
of cases where the slower path is taken for no reason.)
And this in turn would hit _spin_trylock() the same way.
IMO we should harmonize all the trylock variants in this regard:
either they have an as small as possible preemption disabled
section or they have the initial test for success being possible
at all in this section.
Question therefore is how much we care about keeping code
ready for "real" preemption, when we have ample other places
that would need changing first, before such could be enabled.
Yes. And depending on the answer the route to go (wide or narrow
no-preemption section) wither the rwlock or the spinlock trylock
variants should be adapted.
Juergen
|
