Messages in this thread | | | Date | Mon, 15 Nov 2021 17:01:55 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v4] locking/rwsem: Make handoff bit handling more consistent |
| |
On Sun, Nov 14, 2021 at 10:38:57PM -0500, Waiman Long wrote: > On 11/12/21 07:10, Peter Zijlstra wrote:
> > Argh, rwsem_mark_wake() doesn't clear HANDOFF when list_empty(), and > > write_slowpath() is *far* too clever about all of this. > rwsem_mark_wake() does clear the HANDOFF flag if it was set.
Argh, yeah, I got confused by the whole !woken case, but that case won't ever hit list_empty() either. Perhaps that stuff could use a bit of a reflow too.
> > > @@ -1098,7 +1110,7 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state) > > > * In this case, we attempt to acquire the lock again > > > * without sleeping. > > > */ > > > - if (wstate == WRITER_HANDOFF) { > > > + if (waiter.handoff_set) { > > I'm thinking this wants to be something like: > > > > if (rwsem_first_waiter(sem) == &waiter && waiter.handoff_set) { > handoff_set flag is only set when the waiter becomes the first.
Yes, but a random waiter can wake up and see it be set and also start spinning.
> > > > > enum owner_state owner_state; > > > preempt_disable();
> > @@ -575,6 +610,11 @@ static inline bool rwsem_try_write_lock( > > return false; > > } > > + /* > > + * Have rwsem_try_write_lock() fully imply rwsem_del_waiter() on > > + * success. > > + */ > > + list_del(&waiter->list); > > rwsem_set_owner(sem); > > return true; > > }
> > @@ -1128,16 +1153,14 @@ rwsem_down_write_slowpath(struct rw_sema > > raw_spin_lock_irq(&sem->wait_lock); > > } > > __set_current_state(TASK_RUNNING); > > - list_del(&waiter.list); > + rwsem_del_waiter(sem, &waiters); ?
I tried that, but then we get an extra atomic in this path. As is I made try_write_lock() do the full del_waiter, see the hunk above.
> > raw_spin_unlock_irq(&sem->wait_lock); > > lockevent_inc(rwsem_wlock); > > - > > - return ret; > > + return sem; > > out_nolock: > > __set_current_state(TASK_RUNNING); > > raw_spin_lock_irq(&sem->wait_lock); > > - rwsem_out_nolock_clear_flags(sem, &waiter); > > + rwsem_del_waiter(sem, &waiter); > > if (!list_empty(&sem->wait_list)) > > rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q); > > raw_spin_unlock_irq(&sem->wait_lock); > > > Sorry for the late reply as I was busy on other works. > > I like the idea. I will incorporate in a new patch tomorrow.
Thanks!
| |