Re: Wait free LW_SHARED acquisition - v0.2
| От | Amit Kapila |
|---|---|
| Тема | Re: Wait free LW_SHARED acquisition - v0.2 |
| Дата | |
| Msg-id | CAA4eK1+CEjxs9Ye+J3csn8O8DRqNqgLkp0HLJHhq46QNzCEKPw@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: Wait free LW_SHARED acquisition - v0.2 (Andres Freund <andres@2ndquadrant.com>) |
| Ответы |
Re: Wait free LW_SHARED acquisition - v0.2
|
| Список | pgsql-hackers |
On Wed, Oct 22, 2014 at 8:04 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-10-21 19:56:05 +0530, Amit Kapila wrote:
> > On Wed, Oct 8, 2014 at 6:17 PM, Andres Freund <andres@2ndquadrant.com>
> > wrote:
> > spin_delay_count gives
> > how much delay has happened to acquire spinlock which when
> > combined with other stats gives the clear situation about
> > the contention around aquisation of corresponding LWLock.
> > Now if we want to count the spin lock delay for Release call
> > as well, then the meaning of the stat is getting changed.
> > It might be that new meaning of spin_delay_count stat is more
> > useful in some situations, however the older one has its own
> > benefits, so I am not sure if changing this as part of this
> > patch is the best thing to do.
>
> In which case does the old definition make sense, where the new one
> doesn't? I don't think it exists.
>
> And changing it here seems to make sense because spinlock contention
> fundamentally changes it meaning for lwlocks anyway as in most paths we
> don't take a spinlock anymore.
On second thought, I think probably you are right here.
> > > > Why can't we decrement the nwaiters after waking up? I don't think
> > > > there is any major problem even if callers do that themselves, but
> > > > in some rare cases LWLockRelease() might spuriously assume that
> > > > there are some waiters and tries to call LWLockWakeup(). Although
> > > > this doesn't create any problem, keeping the value sane is good unless
> > > > there is some problem in doing so.
> > >
> > > That won't work because then LWLockWakeup() wouldn't be called when
> > > necessary - precisely because nwaiters is 0.
> >
> > > The reason I've done so is that it's otherwise much harder to debug
> > > issues where there are backend that have been woken up already, but
> > > haven't rerun yet. Without this there's simply no evidence of that
> > > state. I can't see this being relevant for performance, so I'd rather
> > > have it stay that way.
> >
> > I am not sure what useful information we can get during debugging by not
> > doing this in LWLockWakeup()
>
> It's useful because you can detect backends that have been scheduled to
> acquire the lock, but haven't yet. They're otherwise "invisible".
>
> > and w.r.t performance it can lead extra
> > function call, few checks and I think in some cases even can
> > acquire/release spinlock.
>
> I fail to see how that could be the case.
> On 2014-10-21 19:56:05 +0530, Amit Kapila wrote:
> > On Wed, Oct 8, 2014 at 6:17 PM, Andres Freund <andres@2ndquadrant.com>
> > wrote:
> > spin_delay_count gives
> > how much delay has happened to acquire spinlock which when
> > combined with other stats gives the clear situation about
> > the contention around aquisation of corresponding LWLock.
> > Now if we want to count the spin lock delay for Release call
> > as well, then the meaning of the stat is getting changed.
> > It might be that new meaning of spin_delay_count stat is more
> > useful in some situations, however the older one has its own
> > benefits, so I am not sure if changing this as part of this
> > patch is the best thing to do.
>
> In which case does the old definition make sense, where the new one
> doesn't? I don't think it exists.
>
> And changing it here seems to make sense because spinlock contention
> fundamentally changes it meaning for lwlocks anyway as in most paths we
> don't take a spinlock anymore.
On second thought, I think probably you are right here.
> > > > Why can't we decrement the nwaiters after waking up? I don't think
> > > > there is any major problem even if callers do that themselves, but
> > > > in some rare cases LWLockRelease() might spuriously assume that
> > > > there are some waiters and tries to call LWLockWakeup(). Although
> > > > this doesn't create any problem, keeping the value sane is good unless
> > > > there is some problem in doing so.
> > >
> > > That won't work because then LWLockWakeup() wouldn't be called when
> > > necessary - precisely because nwaiters is 0.
> >
> > > The reason I've done so is that it's otherwise much harder to debug
> > > issues where there are backend that have been woken up already, but
> > > haven't rerun yet. Without this there's simply no evidence of that
> > > state. I can't see this being relevant for performance, so I'd rather
> > > have it stay that way.
> >
> > I am not sure what useful information we can get during debugging by not
> > doing this in LWLockWakeup()
>
> It's useful because you can detect backends that have been scheduled to
> acquire the lock, but haven't yet. They're otherwise "invisible".
>
> > and w.r.t performance it can lead extra
> > function call, few checks and I think in some cases even can
> > acquire/release spinlock.
>
> I fail to see how that could be the case.
Won't it happen incase first backend sets releaseOK to true and another
backend which tries to wakeup waiters on lock will acquire spinlock
and tries to release the waiters.
> And again, this is code that's
> only executed around a couple syscalls. And the cacheline will be
> touched around there *anyway*.
> >
> >
> > In the above code, if the first waiter to be woken up is Exclusive waiter,
> > then it will woke that waiter, else shared waiters till it got
> > the first exclusive waiter and then first exlusive waiter.
>
> That's would be bug then.
> only executed around a couple syscalls. And the cacheline will be
> touched around there *anyway*.
Sure, but I think syscalls are required in case we need to wake any
waiter.
> >
> >
> > In the above code, if the first waiter to be woken up is Exclusive waiter,
> > then it will woke that waiter, else shared waiters till it got
> > the first exclusive waiter and then first exlusive waiter.
>
> That's would be bug then.
I am not sure of it, but I think it's more important to validate the
new waking startegy as you see benefits by doing so.
>Per the comment you quoted "If the front
> waiter wants exclusive lock, awaken him only. Otherwise awaken as many
> waiters as want shared access.".
>
> But I don't think it's what's happening. Note that 'proc =
> proc->lwWaitLink;' is only executed if 'proc->lwWaitLink->lwWaitMode !=
> LW_EXCLUSIVE'. Which is the next waiter...
>
>
> > > And it'd be a pretty pointless
> > > behaviour, leading to useless increased contention. The only time it'd
> > > make sense for X to be woken up is when it gets run faster than the S
> > > processes.
> >
> > Do we get any major benefit by changing the logic of waking up waiters?
>
> Yes.
> > Code is more readable, but I don't understand why you
> > want to do refactoring as part of this patch which ideally
> > doesn't get any benefit from the same.
>
> I did it first without. But there's required stuff like
> LWLockDequeueSelf(). And I had several bugs because of the list stuff.
>
> And I did separate the conversion into a separate patch?
> waiter wants exclusive lock, awaken him only. Otherwise awaken as many
> waiters as want shared access.".
>
> But I don't think it's what's happening. Note that 'proc =
> proc->lwWaitLink;' is only executed if 'proc->lwWaitLink->lwWaitMode !=
> LW_EXCLUSIVE'. Which is the next waiter...
>
>
> > > And it'd be a pretty pointless
> > > behaviour, leading to useless increased contention. The only time it'd
> > > make sense for X to be woken up is when it gets run faster than the S
> > > processes.
> >
> > Do we get any major benefit by changing the logic of waking up waiters?
>
> Yes.
I think one downside I could see of new strategy is that the chance of
Exclusive waiter to take more time before getting woked up is increased
as now it will by pass Exclusive waiters in queue. I don't have any
concrete proof that it can do any harm to performance, so may be it's
okay to have this new mechanism, however I think it might be helpful
if you could add a comment in code to explain the benefit by skipping
Exclusive lockers.
> > Code is more readable, but I don't understand why you
> > want to do refactoring as part of this patch which ideally
> > doesn't get any benefit from the same.
>
> I did it first without. But there's required stuff like
> LWLockDequeueSelf(). And I had several bugs because of the list stuff.
>
> And I did separate the conversion into a separate patch?
Yeah, but the main patch for wait free LW_SHARED also uses
it.
В списке pgsql-hackers по дате отправления: