public inbox for [email protected]
help / color / mirror / Atom feedFrom: Matthias van de Meent <[email protected]>
To: Andres Freund <[email protected]>
Cc: [email protected]
Cc: Melanie Plageman <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
Date: Sat, 4 Oct 2025 09:05:45 +0200
Message-ID: <CAEze2WgGe8vjj3jiWqUugWuwLJ9cLryaGrnASjm-yJ=tEALX2A@mail.gmail.com> (raw)
In-Reply-To: <6kmid26do57ykqfpvq6iieniy4djsymhrypkjccazq5g4bbe6a@2y6owwv7qpex>
References: <fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff>
<yivb2evcrj7fna5ymuunw3g5u5xxttwjbjxaa4ofkfkviystjv@4dfylftqxyxh>
<6kmid26do57ykqfpvq6iieniy4djsymhrypkjccazq5g4bbe6a@2y6owwv7qpex>
On Tue, 23 Sept 2025 at 00:14, Andres Freund <[email protected]> wrote:
> On 2025-09-15 19:05:37 -0400, Andres Freund wrote:
> > Here are the first few cleaned up patches implementing the above steps, as
> > well as some cleanups. I included a commit from another thread, as it
> > conflicts with these changes, and we really should apply it - and it's
> > arguably required to make the changes viable, as it removes one more use of
> > PinBuffer_Locked().
> >
> > Another change included is to not return the buffer with the spinlock held
> > from StrategyGetBuffer(), and instead pin the buffer in freelist.c. The reason
> > for that is to reduce the most common PinBuffer_locked() call. By definition
> > PinBuffer_locked() will become a bit slower due to 0003. But even without 0003
> > it 0002 is faster than master. And the previous approach also just seems
> > pretty unclean. I don't love that it requires the new TrackNewBufferPin(),
> > but I don't really have a better idea.
> >
> > I invite particular attention to the commit message for 0003 as well as the
> > comment changes in buf_internals.h within.
>
> Robert looked at the patches while we were chatting, and I addressed his
> feedback in this new version.
I like these changes, and have some minor comments:
0001 ensures that ReadRecentBuffer increments the usage counter, which
someone who uses an access strategy may want to prevent. I know this
isn't exactly new behaviour, but something I noticed anyway. Apart
from that observation, LGTM
0002 has a FIXME in a comment in GetVictimBuffer. Assuming it's about
the comment itself needing updates, how about:
+ * Ensure, before we pin a victim buffer, that there's a free refcount
+ * entry, and a resource owner slot for the pin.
Again, LGTM.
0003's UnlockBufHdrExt:
This is implemented with CAS, even when we only want to change bits we
know the state of (or could know, if we spent the effort).
Given its inline nature, wouldn't it be better to use atomic_sub
instructions? Or is this to handle cases where the bits we want to
(un)set might be (un)set by a concurrent process?
If the latter, could we specialize this to do a single atomic_sub
whenever we want to change state bits that we know can be only changed
whilst holding the spinlock?
0004: LGTM
0005: LGTM
0006: LGTM
Kind regards,
Matthias van de Meent
view thread (57+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
In-Reply-To: <CAEze2WgGe8vjj3jiWqUugWuwLJ9cLryaGrnASjm-yJ=tEALX2A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox