Re: Spin Lock sleep resolution

While stracing a process that was contending for a spinlock, I noticed that
the sleep time would often have a longish sequence of 1 and 2 msec sleep
times, rather than the rapidly but randomly increasing sleep time intended.
(At first it looked like it was sleeping on a different attempt each time,
rather than re-sleeping, but some additional logging showed this was not
the case, it was re-sleeping on the same lock attempt.)

The problem is that the state is maintained only to an integer number of
milliseconds starting at 1, so it can take a number of attempts for the
random increment to jump from 1 to 2, and then from 2 to 3.

If the state was instead maintained to an integer number of microseconds
starting at 1000, this granularity of the jump would not be a problem.

And once the state is kept to microseconds, I see no point in not passing
the microsecond precision to pg_usleep.

The attached patch changes the resolution of the state variable to
microseconds, but keeps the starting value at 1msec, i.e. 1000 usec.

Arguably the MIN_DELAY_USEC value should be reduced from 1000 as well given
current hardware, but I'll leave that as a separate issue.

There is the comment:

* The pg_usleep() delays are measured in milliseconds because 1
msec is a
* common resolution limit at the OS level for newer platforms. On
older
* platforms the resolution limit is usually 10 msec, in which case
the
* total delay before timeout will be a bit more.

I think the first sentence is out of date (CentOS 6.3 on "Intel(R) Xeon(R)
CPU E5-2650 0 @ 2.00GHz" not exactly bleeding edge, seems to have a
resolution of 100 usec), and also somewhat bogus (what benefit do we get by
preemptively rounding to some level just because some common platforms will
do so anyway?). I've just removed the whole comment, though maybe some
version of the last sentence needs to be put back.

I have no evidence that the current granularity of the random escalation is
actually causing performance problems, only that is causes confusion
problems for people following along. But I think the change can be
justified based purely on it being cleaner code and more future proof.

This could be made obsolete by changing to futexes, but I doubt that that
is going to happen soon, and certainly not for all platforms.

I will add to commitfest Next

Cheers,

Jeff

Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
> The problem is that the state is maintained only to an integer number of
> milliseconds starting at 1, so it can take a number of attempts for the
> random increment to jump from 1 to 2, and then from 2 to 3.

Hm ... fair point, if you assume that the underlying OS has a sleep
resolution finer than 1ms. Otherwise it would not matter.

> The attached patch changes the resolution of the state variable to
> microseconds, but keeps the starting value at 1msec, i.e. 1000 usec.

No patch seen here ...

regards, tom lane

On Tue, Apr 2, 2013 at 1:24 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
>> The problem is that the state is maintained only to an integer number of
>> milliseconds starting at 1, so it can take a number of attempts for the
>> random increment to jump from 1 to 2, and then from 2 to 3.
>
> Hm ... fair point, if you assume that the underlying OS has a sleep
> resolution finer than 1ms. Otherwise it would not matter.

I would guess it does matter for the cumulative error when re-sleeping
several times.

In any case, the resolution is limited on tick-based kernels, which
are few nowadays. However, I've found evidence[0] that FreeBSD is
still on that boat.

[0] http://lists.freebsd.org/pipermail/freebsd-arch/2012-March/012423.html

On Monday, April 1, 2013, Tom Lane wrote:

> Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
> > The problem is that the state is maintained only to an integer number of
> > milliseconds starting at 1, so it can take a number of attempts for the
> > random increment to jump from 1 to 2, and then from 2 to 3.
>
> Hm ... fair point, if you assume that the underlying OS has a sleep
> resolution finer than 1ms. Otherwise it would not matter.

Let's say you get a long stretch of increments that are all a ratio of <1.5
fold, for simplicity let's say they are all 1.3 fold. When you do
intermediate truncations of the state variable, it never progresses at all.

perl -le '$foo=1; foreach (1..10) {$foo*=1.3; print int $foo}'

perl -le '$foo=1; foreach (1..10) {$foo*=1.3; $foo=int $foo; print int
$foo}'

Obviously the true stochastic case is not quite that stark.

> No patch seen here ...
>

Sorry. I triple checked that the patch was there, but it seems like if you
save a draft with an attachment, when you come back later to finish and
send it, the attachment may not be there anymore. The Gmail Offline teams
still has a ways to go. Hopefully it is actually there this time.

Cheers,

Jeff

On Tue, 2 Apr 2013 09:01:36 -0700
Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:

> Sorry. I triple checked that the patch was there, but it seems like if you
> save a draft with an attachment, when you come back later to finish and
> send it, the attachment may not be there anymore. The Gmail Offline teams
> still has a ways to go. Hopefully it is actually there this time.

I'll give the patch a try, I have a workload that is impacted by spinlocks
fairly heavily sometimes and this might help or at least give me more
information. Thanks!

-dg

--
David Gould daveg(at)sonic(dot)net
If simplicity worked, the world would be overrun with insects.

Hi David,

On 02.04.2013 22:58, David Gould wrote:
> On Tue, 2 Apr 2013 09:01:36 -0700
> Jeff Janes<jeff(dot)janes(at)gmail(dot)com> wrote:
>
>> Sorry. I triple checked that the patch was there, but it seems like if you
>> save a draft with an attachment, when you come back later to finish and
>> send it, the attachment may not be there anymore. The Gmail Offline teams
>> still has a ways to go. Hopefully it is actually there this time.
>
> I'll give the patch a try, I have a workload that is impacted by spinlocks
> fairly heavily sometimes and this might help or at least give me more
> information. Thanks!

Did you ever get around to test this?

I repeated these pgbench tests I did earlier:

http://www.postgresql.org/message-id/5190E17B.9060804@vmware.com

I concluded in that thread that on this platform, the TAS_SPIN macro
really needs a non-locked test before the locked one. That fixes the big
fall in performance with more than 28 clients. So I repeated that test
with four versions:

master - no patch
spin-delay-ms - Jeff's patch
nonlocked-test - master with the non-locked test added to TAS_SPIN
spin-delay-ms-nonlocked-test - both patches

Jeff's patch seems to somewhat alleviate the huge fall in performance
I'm otherwise seeing without the nonlocked-test patch. With the
nonlocked-test patch, if you squint you can see a miniscule benefit.

I wasn't expecting much of a gain from this, just wanted to verify that
it's not making things worse. So looks good to me.

- Heikki

On Tue, 18 Jun 2013 10:09:55 +0300
Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> On 02.04.2013 22:58, David Gould wrote:

> > I'll give the patch a try, I have a workload that is impacted by spinlocks
> > fairly heavily sometimes and this might help or at least give me more
> > information. Thanks!
>
> Did you ever get around to test this?
>
> I repeated these pgbench tests I did earlier:
>
> http://www.postgresql.org/message-id/5190E17B.9060804@vmware.com
>
> I concluded in that thread that on this platform, the TAS_SPIN macro
> really needs a non-locked test before the locked one. That fixes the big
> fall in performance with more than 28 clients.
...
> I wasn't expecting much of a gain from this, just wanted to verify that
> it's not making things worse. So looks good to me.

Thanks for the followup, and I really like your graph, it looks exactly
like what we were hitting. My client ended up configuring around it
and adding more hosts so the urgency to run more tests sort of declined,
although I think we still hit it from time to time.

If you would like to point me at or send me the latest flavor of the patch
it may be timely for me to test again. Especially if this is a more or less
finished version, we are about to roll out a new build to all these hosts
and I'd be happy to try to incorporate this patch and get some production
experience with it on 80 core hosts.

-dg

--
David Gould 510 282 0869 daveg(at)sonic(dot)net
If simplicity worked, the world would be overrun with insects.

On 18.06.2013 10:52, David Gould wrote:
> On Tue, 18 Jun 2013 10:09:55 +0300
> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> wrote:
>
>> I repeated these pgbench tests I did earlier:
>>
>> http://www.postgresql.org/message-id/5190E17B.9060804@vmware.com
>>
>> I concluded in that thread that on this platform, the TAS_SPIN macro
>> really needs a non-locked test before the locked one. That fixes the big
>> fall in performance with more than 28 clients.
> ...
>> I wasn't expecting much of a gain from this, just wanted to verify that
>> it's not making things worse. So looks good to me.
>
> Thanks for the followup, and I really like your graph, it looks exactly
> like what we were hitting. My client ended up configuring around it
> and adding more hosts so the urgency to run more tests sort of declined,
> although I think we still hit it from time to time.

Oh, interesting. What kind of hardware are you running on? To be honest,
I'm not sure what my test hardware is, it's managed by another team
across the world, but /proc/cpuinfo says:

model name : Intel(R) Xeon(R) CPU E5-4640 0 @ 2.40GHz

And it's running in a virtual machine on VMware; that might also be a
factor.

It would be good to test the TAS_SPIN nonlocked patch on a variety of
systems. The comments in s_lock.h say that on Opteron, the non-locked
test is a huge loss. In particular, would be good to re-test that on a
modern AMD system.

> If you would like to point me at or send me the latest flavor of the patch
> it may be timely for me to test again. Especially if this is a more or less
> finished version, we are about to roll out a new build to all these hosts
> and I'd be happy to try to incorporate this patch and get some production
> experience with it on 80 core hosts.

Jeff's patch is here:
http://www.postgresql.org/message-id/CAMkU=1xtP+3UWL6dg10rDRtMA28zy-9ujrOxNuST_r3zOdpNxg@mail.gmail.com.
My non-locked TAS_SPIN patch is the one-liner here:
http://www.postgresql.org/message-id/519A938A.1070903@vmware.com

Thanks!

- Heikki

On Tue, 18 Jun 2013 11:41:06 +0300
Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> Oh, interesting. What kind of hardware are you running on? To be honest,
> I'm not sure what my test hardware is, it's managed by another team
> across the world, but /proc/cpuinfo says:
>
> model name : Intel(R) Xeon(R) CPU E5-4640 0 @ 2.40GHz

It claims to have 80 of these:

model name : Intel(R) Xeon(R) CPU E7-L8867 @2.13GHz

Postgres is on ramfs on these with unlogged tables.

> And it's running in a virtual machine on VMware; that might also be a
> factor.

I'm not a fan of virtualization. It makes performance even harder to
reason about.

> It would be good to test the TAS_SPIN nonlocked patch on a variety of
> systems. The comments in s_lock.h say that on Opteron, the non-locked
> test is a huge loss. In particular, would be good to re-test that on a
> modern AMD system.

I'll see what I can do. However I don't have acces to any large modern AMD
systems.

-dg

--
David Gould 510 282 0869 daveg(at)sonic(dot)net
If simplicity worked, the world would be overrun with insects.

On Tue, Jun 18, 2013 at 12:09 AM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)com> wrote:

>
> Jeff's patch seems to somewhat alleviate the huge fall in performance I'm
> otherwise seeing without the nonlocked-test patch. With the nonlocked-test
> patch, if you squint you can see a miniscule benefit.
>
> I wasn't expecting much of a gain from this, just wanted to verify that
> it's not making things worse. So looks good to me.

Hi Heikki,

Thanks for trying out the patch.

I see in the commitfest app it is set to "Waiting on Author" (but I don't
know who "maiku41" is).

Based on the comments so far, I don't know what I should be doing on it at
the moment, and I thought perhaps your comment above meant it should be
"ready for committer".

If we think the patch has a risk of introducing subtle errors, then it
probably can't be justified based on the small performance gains you saw.

But if we think it has little risk, then I think it is justified simply
based on cleaner code, and less confusion for people who are tracing a
problematic process. If we want it to do a random escalation, it should
probably look like a random escalation to the interested observer.

Thanks,

Jeff

Jeff Janes escribió:

> I see in the commitfest app it is set to "Waiting on Author" (but I don't
> know who "maiku41" is).

Yeah, that guy is misterious. I'm guessing the Mike Blackwell person
Josh mentioned in his "week 1" report.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 06/26/2013 02:49 PM, Jeff Janes wrote:
> I see in the commitfest app it is set to "Waiting on Author" (but I don't
> know who "maiku41" is).

Mike Blackwell, who's helping track patches for the CommitFest.

It's been our practice since the 9.3 cycle that patches which are still
under contentious discussion which blocks commit are "waiting on author"
rather than some other status. It was not clear to me (or to Mike) that
the dicussion on this patch was concluded favorably. Is it?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Jun 26, 2013 at 5:49 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> On Tue, Jun 18, 2013 at 12:09 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> Jeff's patch seems to somewhat alleviate the huge fall in performance I'm
>> otherwise seeing without the nonlocked-test patch. With the nonlocked-test
>> patch, if you squint you can see a miniscule benefit.
>>
>> I wasn't expecting much of a gain from this, just wanted to verify that
>> it's not making things worse. So looks good to me.
>
> Hi Heikki,
>
> Thanks for trying out the patch.
>
> I see in the commitfest app it is set to "Waiting on Author" (but I don't
> know who "maiku41" is).
>
> Based on the comments so far, I don't know what I should be doing on it at
> the moment, and I thought perhaps your comment above meant it should be
> "ready for committer".
>
> If we think the patch has a risk of introducing subtle errors, then it
> probably can't be justified based on the small performance gains you saw.
>
> But if we think it has little risk, then I think it is justified simply
> based on cleaner code, and less confusion for people who are tracing a
> problematic process. If we want it to do a random escalation, it should
> probably look like a random escalation to the interested observer.

I think it has little risk. If it turns out to be worse for
performance, we can always revert it, but I expect it'll be better or
a wash, and the results so far seem to bear that out. Another
interesting question is whether we should fool with the actual values
for minimum and maximum delays, but that's a separate and much harder
question, so I think we should just do this for now and call it good.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 27.06.2013 17:30, Robert Haas wrote:
> On Wed, Jun 26, 2013 at 5:49 PM, Jeff Janes<jeff(dot)janes(at)gmail(dot)com> wrote:
>> If we think the patch has a risk of introducing subtle errors, then it
>> probably can't be justified based on the small performance gains you saw.
>>
>> But if we think it has little risk, then I think it is justified simply
>> based on cleaner code, and less confusion for people who are tracing a
>> problematic process. If we want it to do a random escalation, it should
>> probably look like a random escalation to the interested observer.
>
> I think it has little risk. If it turns out to be worse for
> performance, we can always revert it, but I expect it'll be better or
> a wash, and the results so far seem to bear that out. Another
> interesting question is whether we should fool with the actual values
> for minimum and maximum delays, but that's a separate and much harder
> question, so I think we should just do this for now and call it good.

My thoughts exactly. I wanted to see if David Gould would like to do
some testing with it, but there's realy no need to hold off committing
for that, I'm not expecting any surprises there. Committed.

Jeff, in the patch you changed the datatype of cur_delay variable from
int to long. AFAICS that makes no difference, so I kept it as int. Let
me know if there was a reason for that change.

- Heikki

On Fri, Jun 28, 2013 at 2:46 AM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
> wrote:

> On 27.06.2013 17:30, Robert Haas wrote:
>
>> On Wed, Jun 26, 2013 at 5:49 PM, Jeff Janes<jeff(dot)janes(at)gmail(dot)com> wrote:
>>
>>> If we think the patch has a risk of introducing subtle errors, then it
>>>
>>> probably can't be justified based on the small performance gains you saw.
>>>
>>> But if we think it has little risk, then I think it is justified simply
>>> based on cleaner code, and less confusion for people who are tracing a
>>> problematic process. If we want it to do a random escalation, it should
>>> probably look like a random escalation to the interested observer.
>>>
>>
>> I think it has little risk. If it turns out to be worse for
>> performance, we can always revert it, but I expect it'll be better or
>> a wash, and the results so far seem to bear that out. Another
>> interesting question is whether we should fool with the actual values
>> for minimum and maximum delays, but that's a separate and much harder
>> question, so I think we should just do this for now and call it good.
>>
>
> My thoughts exactly. I wanted to see if David Gould would like to do some
> testing with it, but there's realy no need to hold off committing for that,
> I'm not expecting any surprises there. Committed.
>

Thanks.

>
> Jeff, in the patch you changed the datatype of cur_delay variable from int
> to long. AFAICS that makes no difference, so I kept it as int. Let me know
> if there was a reason for that change.

I think my thought process at the time was that since the old code
multiplied by "1000L", not "1000" in the expression argument to pg_usleep,
I wanted to keep the spirit of the L in place. It seemed safer than trying
to prove to myself that it was not necessary.

If int suffices, should the L come out of the defines for MIN_DELAY_USEC
and MAX_DELAY_USEC ?

Cheers,

Jeff