Re: StrategyGetBuffer optimization, take 2

On Tue, Aug 20, 2013 at 1:57 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-08-19 15:17:44 -0700, Jeff Janes wrote:
>> On Wed, Aug 7, 2013 at 7:40 AM, Merlin Moncure <mmoncure@gmail.com> wrote:
>>
>> > I agree; at least then it's not unambiguously better. if you (in
>> > effect) swap all contention on allocation from a lwlock to a spinlock
>> > it's not clear if you're improving things; it would have to be proven
>> > and I'm trying to keep things simple.
>> >
>> > Attached is a scaled down version of the patch that keeps the freelist
>> > lock but still removes the spinlock during the clock sweep.  This
>> > still hits the major objectives of reducing the chance of scheduling
>> > out while holding the BufFreelistLock and mitigating the worst case
>> > impact of doing so if it does happen.  An even more scaled down
>> > version would keep the current logic exactly as is except for
>> > replacing buffer lock in the clock sweep with a trylock (which is
>> > IMNSHO a no-brainer).
>>
>> Since usage_count is unsigned, are you sure that changing the tests
>> from "buf->usage_count == 0" to "buf->usage_count <= 0" accomplishes
>> what you need it to?  If usage_count gets decremented when it already
>> zero, it will wrap around to 65,535, at least on some compilers some
>> of the time, won't it?
>
> Overflow of *unsigned* variables is actually defined and will always
> wrap around. It's signed variables which don't have such a clear
> behaviour.


Hm, well, even better would be to leave things as they are and try to
guarantee that usage_count is updated via assignment vs increment;
that way it would be impossible to wander out of bounds.  I bet
changing:
i--; to i=(i-1);

isn't going to do much against modern compilers.  But what about
assignment from a volatile temporary?

volatile v = usage_count;
if (v > 0) v--;
usage_count = v;

something like that.  Or maybe declaring usage_count as volatile might
be enough?

merlin