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

Search Discussions

  • Heikki Linnakangas at Jan 4, 2012 at 7:24 am

    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
  • Simon Riggs at Jan 4, 2012 at 8:19 am

    On Wed, Jan 4, 2012 at 7:24 AM, Heikki Linnakangas 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
  • Peter Geoghegan at Jan 4, 2012 at 3:05 pm

    On 4 January 2012 07:24, Heikki Linnakangas 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
  • Heikki Linnakangas at Jan 17, 2012 at 10:16 am

    On 04.01.2012 17:05, Peter Geoghegan wrote:
    On 4 January 2012 07:24, Heikki Linnakangas
    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
  • Heikki Linnakangas at Jan 17, 2012 at 11:25 am

    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
    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
  • Peter Geoghegan at Jan 17, 2012 at 12:38 pm

    On 17 January 2012 11:24, Heikki Linnakangas 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
  • Heikki Linnakangas at Jan 26, 2012 at 4:48 pm

    On 17.01.2012 14:38, Peter Geoghegan wrote:
    On 17 January 2012 11:24, Heikki Linnakangas
    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
  • Peter Geoghegan at Jan 26, 2012 at 7:37 pm

    On 26 January 2012 16:48, Heikki Linnakangas 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
  • Heikki Linnakangas at Jan 27, 2012 at 7:17 am

    On 26.01.2012 21:37, Peter Geoghegan wrote:
    On 26 January 2012 16:48, Heikki Linnakangas
    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedJan 4, '12 at 5:58a
activeJan 27, '12 at 7:17a
posts10
users3
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase