> > >
> > > + if (READ_ONCE(sem->owner))
> > > + return true; /* new owner, continue spinning */
> > > +
> >
> > Do you have some comparison data of whether it is more advantageous
> > to continue spinning when owner changes? After the above change,
> > rwsem will behave more like a spin lock for write lock and
> > will keep spinning when the lock changes ownership.
>
> But recall we still abort when need_resched, so the spinning isn't
> infinite. Never has been.
>
> > Now during heavy
> > lock contention, if we don't continue spinning and sleep, we may use the
> > clock cycles for actually running other threads.
>
> Under heavy contention, time spinning will force us to ultimately block
> anyway.
The question is under heavy contention, if we are going to block anyway,
won't it be more advantageous not to continue spinning so we can use
the cycles for useful task? The original code assumes that if the lock
has switched owner, then we are under heavy contention and we can stop
spinning and block. I think it'll be useful to have some
data comparing the two behaviors.
Thanks.
Tim
On Tue, 2015-02-03 at 09:16 -0800, Tim Chen wrote:
> > > >
> > > > + if (READ_ONCE(sem->owner))
> > > > + return true; /* new owner, continue spinning */
> > > > +
> > >
> > > Do you have some comparison data of whether it is more advantageous
> > > to continue spinning when owner changes? After the above change,
> > > rwsem will behave more like a spin lock for write lock and
> > > will keep spinning when the lock changes ownership.
> >
> > But recall we still abort when need_resched, so the spinning isn't
> > infinite. Never has been.
> >
> > > Now during heavy
> > > lock contention, if we don't continue spinning and sleep, we may use the
> > > clock cycles for actually running other threads.
> >
> > Under heavy contention, time spinning will force us to ultimately block
> > anyway.
>
> The question is under heavy contention, if we are going to block anyway,
> won't it be more advantageous not to continue spinning so we can use
> the cycles for useful task?
Hi Tim,
Now that we have the OSQ logic, under heavy contention, there will still
only be 1 thread that is spinning on owner at a time. So if another
thread is able to obtain the lock before the spinner, we're only sending
the top spinner of the lock to the slowpath. As long as the new lock
owner is running, there is a chance for this top spinner to obtain the
lock, and spinning would be useful.
Since we have the need_resched() checks, this thread will block when
there really is another task that should run over the spinning thread.
On Tue, 2015-02-03 at 09:54 -0800, Jason Low wrote:
> On Tue, 2015-02-03 at 09:16 -0800, Tim Chen wrote:
> > > > >
> > > > > + if (READ_ONCE(sem->owner))
> > > > > + return true; /* new owner, continue spinning */
> > > > > +
> > > >
> > > > Do you have some comparison data of whether it is more advantageous
> > > > to continue spinning when owner changes? After the above change,
> > > > rwsem will behave more like a spin lock for write lock and
> > > > will keep spinning when the lock changes ownership.
> > >
> > > But recall we still abort when need_resched, so the spinning isn't
> > > infinite. Never has been.
> > >
> > > > Now during heavy
> > > > lock contention, if we don't continue spinning and sleep, we may use the
> > > > clock cycles for actually running other threads.
> > >
> > > Under heavy contention, time spinning will force us to ultimately block
> > > anyway.
> >
> > The question is under heavy contention, if we are going to block anyway,
> > won't it be more advantageous not to continue spinning so we can use
> > the cycles for useful task?
>
> Hi Tim,
>
> Now that we have the OSQ logic, under heavy contention, there will still
> only be 1 thread that is spinning on owner at a time.
That's true. We cannot have the lock grabbed by a new write
contender as any new writer contender of the lock will be
queued by the OSQ logic. Only the
thread doing the optimistic spin is attempting write lock.
In other word, switching of write owner of the rwsem to a new
owner cannot happen. Either write owner stay as the original one, or
we don't have a write owner. So using test of write owner
switching as an indicator of congestion is incorrect.
If my reasoning above is sound, then the check
+ if (READ_ONCE(sem->owner))
+ return true; /* new owner, continue spinning */
+
is unnecessary and can be removed, as we cannot have a
new write owner of the rwsem, other than the thread
doing optimistic spinning.
Tim
On Tue, 2015-02-03 at 11:43 -0800, Tim Chen wrote:
> On Tue, 2015-02-03 at 09:54 -0800, Jason Low wrote:
> > On Tue, 2015-02-03 at 09:16 -0800, Tim Chen wrote:
> > > > > >
> > > > > > + if (READ_ONCE(sem->owner))
> > > > > > + return true; /* new owner, continue spinning */
> > > > > > +
> > > > >
> > > > > Do you have some comparison data of whether it is more advantageous
> > > > > to continue spinning when owner changes? After the above change,
> > > > > rwsem will behave more like a spin lock for write lock and
> > > > > will keep spinning when the lock changes ownership.
> > > >
> > > > But recall we still abort when need_resched, so the spinning isn't
> > > > infinite. Never has been.
> > > >
> > > > > Now during heavy
> > > > > lock contention, if we don't continue spinning and sleep, we may use the
> > > > > clock cycles for actually running other threads.
> > > >
> > > > Under heavy contention, time spinning will force us to ultimately block
> > > > anyway.
> > >
> > > The question is under heavy contention, if we are going to block anyway,
> > > won't it be more advantageous not to continue spinning so we can use
> > > the cycles for useful task?
> >
> > Hi Tim,
> >
> > Now that we have the OSQ logic, under heavy contention, there will still
> > only be 1 thread that is spinning on owner at a time.
>
> That's true. We cannot have the lock grabbed by a new write
> contender as any new writer contender of the lock will be
> queued by the OSQ logic. Only the
> thread doing the optimistic spin is attempting write lock.
> In other word, switching of write owner of the rwsem to a new
> owner cannot happen.
Another thread can still obtain the write lock in the fast path though
right? We try to obtain the write lock once before calling
rwsem_down_write_failed().
On Tue, 2015-02-03 at 13:04 -0800, Jason Low wrote:
> On Tue, 2015-02-03 at 11:43 -0800, Tim Chen wrote:
> > On Tue, 2015-02-03 at 09:54 -0800, Jason Low wrote:
> > > On Tue, 2015-02-03 at 09:16 -0800, Tim Chen wrote:
> > > > > > >
> > > > > > > + if (READ_ONCE(sem->owner))
> > > > > > > + return true; /* new owner, continue spinning */
> > > > > > > +
> > > > > >
> > > > > > Do you have some comparison data of whether it is more advantageous
> > > > > > to continue spinning when owner changes? After the above change,
> > > > > > rwsem will behave more like a spin lock for write lock and
> > > > > > will keep spinning when the lock changes ownership.
> > > > >
> > > > > But recall we still abort when need_resched, so the spinning isn't
> > > > > infinite. Never has been.
> > > > >
> > > > > > Now during heavy
> > > > > > lock contention, if we don't continue spinning and sleep, we may use the
> > > > > > clock cycles for actually running other threads.
> > > > >
> > > > > Under heavy contention, time spinning will force us to ultimately block
> > > > > anyway.
> > > >
> > > > The question is under heavy contention, if we are going to block anyway,
> > > > won't it be more advantageous not to continue spinning so we can use
> > > > the cycles for useful task?
> > >
> > > Hi Tim,
> > >
> > > Now that we have the OSQ logic, under heavy contention, there will still
> > > only be 1 thread that is spinning on owner at a time.
> >
> > That's true. We cannot have the lock grabbed by a new write
> > contender as any new writer contender of the lock will be
> > queued by the OSQ logic. Only the
> > thread doing the optimistic spin is attempting write lock.
> > In other word, switching of write owner of the rwsem to a new
> > owner cannot happen.
>
> Another thread can still obtain the write lock in the fast path though
> right? We try to obtain the write lock once before calling
> rwsem_down_write_failed().
>
>
True. The change owner check is still needed then.
Thinking more about this, I now agree that continue spinning is the
right thing. The possible number of threads contending for write
locking has been greatly reduced by OSQ logic. Most of the time
any new threads doing write locking attempt will do that only once
and then go directly to the OSQ. The probability of success of retrying
write lock by the thread at head of OSQ is high so we should do it.
Davidlohr, you can add my Ack for this patch.
Thanks.
Tim
On Tue, Feb 03, 2015 at 11:43:36AM -0800, Tim Chen wrote:
> That's true. We cannot have the lock grabbed by a new write
> contender as any new writer contender of the lock will be
> queued by the OSQ logic. Only the
> thread doing the optimistic spin is attempting write lock.
> In other word, switching of write owner of the rwsem to a new
> owner cannot happen. Either write owner stay as the original one, or
> we don't have a write owner. So using test of write owner
> switching as an indicator of congestion is incorrect.
>
> If my reasoning above is sound, then the check
>
> + if (READ_ONCE(sem->owner))
> + return true; /* new owner, continue spinning */
> +
>
> is unnecessary and can be removed, as we cannot have a
> new write owner of the rwsem, other than the thread
> doing optimistic spinning.
I have read the rest of the thread; but the one thing that I didn't see
is trylocks, trylocks can always come in an steal things regardless of
the OSQ stuff.
On Wed, 2015-02-04 at 13:06 +0100, Peter Zijlstra wrote:
> On Tue, Feb 03, 2015 at 11:43:36AM -0800, Tim Chen wrote:
> > That's true. We cannot have the lock grabbed by a new write
> > contender as any new writer contender of the lock will be
> > queued by the OSQ logic. Only the
> > thread doing the optimistic spin is attempting write lock.
> > In other word, switching of write owner of the rwsem to a new
> > owner cannot happen. Either write owner stay as the original one, or
> > we don't have a write owner. So using test of write owner
> > switching as an indicator of congestion is incorrect.
> >
> > If my reasoning above is sound, then the check
> >
> > + if (READ_ONCE(sem->owner))
> > + return true; /* new owner, continue spinning */
> > +
> >
> > is unnecessary and can be removed, as we cannot have a
> > new write owner of the rwsem, other than the thread
> > doing optimistic spinning.
>
> I have read the rest of the thread; but the one thing that I didn't see
> is trylocks, trylocks can always come in an steal things regardless of
> the OSQ stuff.
Jason also pointed that out. So the owner change check is needed
after all. Now because of the OSQ logic, even if owner has changed,
the likelihood that the spinner at the head of OSQ will acquire the
lock is high. So it should continue to spin.
That's because any new threads coming in will try lock only
once, and go to the OSQ. It is unlikely that they will trylock at
the precise moment when the owner release the lock as they do not
continue to spin on the lock. The contention from new threads
are low.
So letting the thread at head of OSQ to continue to spin is probably
the right thing to do.
Tim