Re: BGWriter latch, power saving

As part of the ongoing effort to reduce wake-ups when idle/power
consumption, the attached patch modifies the background writer to
hibernate in ten second bursts once the bgwriter laps the clock sweep.
It's fairly well commented, so a description of how it works here
would probably be redundant. The most controversial aspect of this
patch is the fact that it adds a SetLatch() call to MarkBufferDirty(),
though I went to some lengths to ensure that that call will very
probably find the latch already set when it actually matters, so it
only checks a single flag.

Thoughts? I can produce benchmarks, if that helps, but I think it's
fairly unlikely that the patch introduces a measurable performance
regression.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 04.01.2012 07:58, Peter Geoghegan wrote:
> As part of the ongoing effort to reduce wake-ups when idle/power
> consumption, the attached patch modifies the background writer to
> hibernate in ten second bursts once the bgwriter laps the clock sweep.
> It's fairly well commented, so a description of how it works here
> would probably be redundant. The most controversial aspect of this
> patch is the fact that it adds a SetLatch() call to MarkBufferDirty(),
> though I went to some lengths to ensure that that call will very
> probably find the latch already set when it actually matters, so it
> only checks a single flag.

I think SetBufferCommitInfoNeedsSave() needs the same treatment as
MarkBufferDirty(). And it would probably be good to only set the latch
if the buffer wasn't dirty already. Setting a latch that's already set
is fast, but surely it's even faster to not even try.

> Thoughts? I can produce benchmarks, if that helps, but I think it's
> fairly unlikely that the patch introduces a measurable performance
> regression.

Yeah, I'd like to see a micro-benchmark of a worst-case scenario. I'm a
bit worried about the impact on systems with a lot of CPUs. If you have
a lot of CPUs writing to the same cache line that contains the latch's
flag, that might get expensive.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, Jan 4, 2012 at 7:24 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> Setting a latch that's already set is fast,
> but surely it's even faster to not even try.

Agreed. I think we should SetLatch() at the first point a backend
writes a dirty buffer because the bgwriter has been inactive.

Continually waking the bgwriter makes it harder for it to return to sleep.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 4 January 2012 07:24, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> I think SetBufferCommitInfoNeedsSave() needs the same treatment as
> MarkBufferDirty(). And it would probably be good to only set the latch if
> the buffer wasn't dirty already. Setting a latch that's already set is fast,
> but surely it's even faster to not even try.

That seems reasonable. Revised patch is attached.

> Yeah, I'd like to see a micro-benchmark of a worst-case scenario. I'm a bit
> worried about the impact on systems with a lot of CPUs. If you have a lot of
> CPUs writing to the same cache line that contains the latch's flag, that
> might get expensive.

Also reasonable, but I don't think that I'll get around to it until
after the final commitfest deadline.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 04.01.2012 17:05, Peter Geoghegan wrote:
> On 4 January 2012 07:24, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> I think SetBufferCommitInfoNeedsSave() needs the same treatment as
>> MarkBufferDirty(). And it would probably be good to only set the latch if
>> the buffer wasn't dirty already. Setting a latch that's already set is fast,
>> but surely it's even faster to not even try.
>
> That seems reasonable. Revised patch is attached.

Thanks! It occurs to me that it's still a bad idea to call SetLatch()
while holding the buffer header spinlock. At least when it's trivial to
just move the call after releasing the lock. See attached.

Could you do the sleeping/hibernating logic all in BgWriterNap()?

>> Yeah, I'd like to see a micro-benchmark of a worst-case scenario. I'm a bit
>> worried about the impact on systems with a lot of CPUs. If you have a lot of
>> CPUs writing to the same cache line that contains the latch's flag, that
>> might get expensive.
>
> Also reasonable, but I don't think that I'll get around to it until
> after the final commitfest deadline.

That's still pending. And I admit I haven't tested my updated version
besides "make check".

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 17.01.2012 12:16, Heikki Linnakangas wrote:
> On 04.01.2012 17:05, Peter Geoghegan wrote:
>> On 4 January 2012 07:24, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> I think SetBufferCommitInfoNeedsSave() needs the same treatment as
>>> MarkBufferDirty(). And it would probably be good to only set the
>>> latch if
>>> the buffer wasn't dirty already. Setting a latch that's already set
>>> is fast,
>>> but surely it's even faster to not even try.
>>
>> That seems reasonable. Revised patch is attached.
>
> Thanks! It occurs to me that it's still a bad idea to call SetLatch()
> while holding the buffer header spinlock. At least when it's trivial to
> just move the call after releasing the lock. See attached.
>
> Could you do the sleeping/hibernating logic all in BgWriterNap()?

(sorry, forgot to update the above question before sending..)

In the patch I sent, I did rearrange the sleeping logic. I think it's
more readable the way it is now.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 17 January 2012 11:24, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> In the patch I sent, I did rearrange the sleeping logic. I think it's more
> readable the way it is now.

I have no objection to either your refinement of the sleeping logic,
nor that you moved some things in both the existing code and my patch
so that they occur when no spinlock is held.

Should I proceed with a benchmark on V3, so that we can get this
committed? I imagine that a long pgbench-tools run is appropriate,
(after all, it was used to justify the re-write of the BGWriter for
8.3) at various scale factors, from smallish to quite large.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 17.01.2012 14:38, Peter Geoghegan wrote:
> On 17 January 2012 11:24, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> In the patch I sent, I did rearrange the sleeping logic. I think it's more
>> readable the way it is now.
>
> I have no objection to either your refinement of the sleeping logic,
> nor that you moved some things in both the existing code and my patch
> so that they occur when no spinlock is held.

Ok, committed with some further cleanup.

Do you think the docs need to be updated for this, and if so, where? The
only place I found in the docs that speak about how the bgwriter works
is in config.sgml, where bgwriter_delay is described. Want to suggest an
update to that?

> Should I proceed with a benchmark on V3, so that we can get this
> committed? I imagine that a long pgbench-tools run is appropriate,
> (after all, it was used to justify the re-write of the BGWriter for
> 8.3) at various scale factors, from smallish to quite large.

I did some testing on this, with a highly artificial test case that
dirties pages in shared_buffers as fast as possible. I tried to make it
a worst-case scenario, see attached script. I tested this with a 32-core
HP Itanium box, and on my 2-core laptop, and didn't see any measurable
slowdown from this patch. So I think we're good.

If setting the latch would become a contention issue, there would be a
pretty easy solution: only try to do it every 10 or 100 dirtied pages,
for example. A few dirty pages in the buffer cache don't mean anything,
as long as we kick the bgwriter in a fairly timely fashion when a larger
burst of activity begins.

BTW, do you have some sort of a test setup for these power-saving
patches, to actually measure the effect on number of interrupts or
electricity use? Fewer wakeups should be a good thing, but it would be
nice to see some figures to get an idea of how much progress we've done
and what still needs to be done.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 26 January 2012 16:48, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> Ok, committed with some further cleanup.
>
> Do you think the docs need to be updated for this, and if so, where? The
> only place I found in the docs that speak about how the bgwriter works is in
> config.sgml, where bgwriter_delay is described. Want to suggest an update to
> that?

This new behaviour might warrant a mention, as in the attached doc-patch.

> I did some testing on this, with a highly artificial test case that dirties
> pages in shared_buffers as fast as possible. I tried to make it a worst-case
> scenario, see attached script. I tested this with a 32-core HP Itanium box,
> and on my 2-core laptop, and didn't see any measurable slowdown from this
> patch. So I think we're good.

Cool. I was pretty confident that it would be completely impossible to
show a regression under any circumstances, but I'm glad that you
tested it on a large server like that.

> BTW, do you have some sort of a test setup for these power-saving patches,
> to actually measure the effect on number of interrupts or electricity use?
> Fewer wakeups should be a good thing, but it would be nice to see some
> figures to get an idea of how much progress we've done and what still needs
> to be done.

The only thing I've been using is Intel's powertop utility. It is
pretty easy to see what's happening, and what you'll see is exactly
what you'd expect (So by process, I could see that the bgwriter had
exactly 5 wake-ups per second with bgwriter_delay at 200). It is
useful to sleep quite pro-actively, as CPUs will enter idle C-states
and move to lower P-states quite quickly (some will be more familiar
with the commercial names for P-states, such as Intel's speedstep
technology). I might have been less conservative about the
circumstances that cause the bgwriter to go to sleep, but I was
conscious of the need to get something into this release.

It's difficult to know what a useful workload should be to show the
benefits of this work, apart from the obvious one, which is to show
Postgres when completely idle. I think we started at 11.5 wake-ups per
second, and I think that's down to about 6.5 now - the WAL Writer
still accounts for 5 of those, so that's why I feel it's particularly
important to get it done too, though obviously that's something that
should defer to however we end up implementing group commit.

I intend to blog about it in the next few days, and I'll present a
careful analysis of the benefits of this work there. Look out for it
on planet.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 26.01.2012 21:37, Peter Geoghegan wrote:
> On 26 January 2012 16:48, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> Ok, committed with some further cleanup.
>>
>> Do you think the docs need to be updated for this, and if so,
>> where? The only place I found in the docs that speak about how the
>> bgwriter works is in config.sgml, where bgwriter_delay is
>> described. Want to suggest an update to that?
>
> This new behaviour might warrant a mention, as in the attached
> doc-patch.

Hmm, the word "hibernate" might confuse people in this context. I
committed this as:

> It then sleeps for <varname>bgwriter_delay</> milliseconds, and
> repeats. When there are no dirty buffers in the buffer pool, though,
> it goes into a longer sleep regardless of <varname>bgwriter_delay</>.

Thanks!

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com