Switching TestSetPageLocked to trylock_page

Switching TestSetPageLocked to trylock_page

[Nick Piggin]

Hi,

I'm wondering if I could get a patch merged which changes all
TestSetPageLocked and replaces them with trylock_page?

It would be done so there are no functional changes.

The reason I want to (aside from having a slightly more
intuitive API), is to facilitate the implementation of lockdep
on page lock, and also to use lock semantics bitops for the
lock rather than the big-hammer barriers that come with
test_and_set_bit.

The lock bitops stuff is one of the parts of my patchset to speed
up page lock functions (which results in nearly 50% faster
pagecache throughput on my G5, and even slightly faster on x86)...

Anyway, I remember you said these kinds of changes are appropriate
for just after -rc1, and I agree it shouldn't cause much pain.

Fixing up patch conflicts is literally a matter of
s/!TestSetPageLocked/trylock_page
s/TestSetPageLocked/!trylock_page

Thoughts?

Re: Switching TestSetPageLocked to trylock_page

[KOSAKI Motohiro]

Hi!

> I'm wondering if I could get a patch merged which changes all
> TestSetPageLocked and replaces them with trylock_page?

ACK ACK ACK

I don't read this patch detail.
but it seems good idea.

Re: Switching TestSetPageLocked to trylock_page

[Peter Zijlstra]

On Thu, 2008-07-31 at 17:26 +1000, Nick Piggin wrote:
> Hi,
> 
> I'm wondering if I could get a patch merged which changes all
> TestSetPageLocked and replaces them with trylock_page?
> 
> It would be done so there are no functional changes.
> 
> The reason I want to (aside from having a slightly more
> intuitive API), is to facilitate the implementation of lockdep
> on page lock, and also to use lock semantics bitops for the
> lock rather than the big-hammer barriers that come with
> test_and_set_bit.
> 
> The lock bitops stuff is one of the parts of my patchset to speed
> up page lock functions (which results in nearly 50% faster
> pagecache throughput on my G5, and even slightly faster on x86)...
> 
> Anyway, I remember you said these kinds of changes are appropriate
> for just after -rc1, and I agree it shouldn't cause much pain.
> 
> Fixing up patch conflicts is literally a matter of
> s/!TestSetPageLocked/trylock_page
> s/TestSetPageLocked/!trylock_page
> 
> Thoughts?

I'm all for it ;-)

Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin]

On Thursday 31 July 2008 17:26, Nick Piggin wrote:
> Hi,
>
> I'm wondering if I could get a patch merged which changes all
> TestSetPageLocked and replaces them with trylock_page?

Yes? No?

The alternative is try to merge it via -mm or -next, but that just
wastes everybodies time with conflicts of having these differences
between -mm and mainline.


>
> It would be done so there are no functional changes.
>
> The reason I want to (aside from having a slightly more
> intuitive API), is to facilitate the implementation of lockdep
> on page lock, and also to use lock semantics bitops for the
> lock rather than the big-hammer barriers that come with
> test_and_set_bit.
>
> The lock bitops stuff is one of the parts of my patchset to speed
> up page lock functions (which results in nearly 50% faster
> pagecache throughput on my G5, and even slightly faster on x86)...
>
> Anyway, I remember you said these kinds of changes are appropriate
> for just after -rc1, and I agree it shouldn't cause much pain.
>
> Fixing up patch conflicts is literally a matter of
> s/!TestSetPageLocked/trylock_page
> s/TestSetPageLocked/!trylock_page
>
> Thoughts?

Re: Switching TestSetPageLocked to trylock_page

[Linus Torvalds]

On Tue, 5 Aug 2008, Nick Piggin wrote:

> On Thursday 31 July 2008 17:26, Nick Piggin wrote:
> > Hi,
> >
> > I'm wondering if I could get a patch merged which changes all
> > TestSetPageLocked and replaces them with trylock_page?
> 
> Yes? No?
> 
> The alternative is try to merge it via -mm or -next, but that just
> wastes everybodies time with conflicts of having these differences
> between -mm and mainline.

Heh. I had just been _assuming_ this would go through -mm, since it's 
exactly the kind of thing that usually does go through there.

So I hadn't even really considered it.

I don't mind the patch per se, but can you give some background on what 
the pending optimization is that makes such a big difference?

		Linus

Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin]

On Tuesday 05 August 2008 13:28, Linus Torvalds wrote:
> On Tue, 5 Aug 2008, Nick Piggin wrote:
> > On Thursday 31 July 2008 17:26, Nick Piggin wrote:
> > > Hi,
> > >
> > > I'm wondering if I could get a patch merged which changes all
> > > TestSetPageLocked and replaces them with trylock_page?
> >
> > Yes? No?
> >
> > The alternative is try to merge it via -mm or -next, but that just
> > wastes everybodies time with conflicts of having these differences
> > between -mm and mainline.
>
> Heh. I had just been _assuming_ this would go through -mm, since it's
> exactly the kind of thing that usually does go through there.
>
> So I hadn't even really considered it.

OK... it just causes Andrew headaches I suspect. But if he prefers
to hold onto it for an entire release cycle... Andrew?


> I don't mind the patch per se, but can you give some background on what
> the pending optimization is that makes such a big difference?

Using the lock semantics bitops is the first one. While it is true
that we could just hack them into TestSetPageLocked, I really prefer
callers to require at least a cursory glance to convert them, and
understand that this is a lock lock, and not a test_and_set bitop
with full barrier semantics.

lock semantics bitops obviously doesn't help x86 a bit. It really
helps powerpc though.

The part that helps x86 is another patch further down my stack, which
can avoid looking at the hashed page waitqueue at unlock_page-time.
Basically this eliminates the entire page waitqueue from the cache
footprint of fastpath workloads where the lock is uncontended (often:
page faulting, write(2), writeout etc).

I'll send all that stuff off to -mm for 2.6.28 if this gets upstream
(or into -mm).

Re: Switching TestSetPageLocked to trylock_page

[Andrew Morton]

On Tue, 5 Aug 2008 13:41:53 +1000 Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> On Tuesday 05 August 2008 13:28, Linus Torvalds wrote:
> > On Tue, 5 Aug 2008, Nick Piggin wrote:
> > > On Thursday 31 July 2008 17:26, Nick Piggin wrote:
> > > > Hi,
> > > >
> > > > I'm wondering if I could get a patch merged which changes all
> > > > TestSetPageLocked and replaces them with trylock_page?
> > >
> > > Yes? No?
> > >
> > > The alternative is try to merge it via -mm or -next, but that just
> > > wastes everybodies time with conflicts of having these differences
> > > between -mm and mainline.
> >
> > Heh. I had just been _assuming_ this would go through -mm, since it's
> > exactly the kind of thing that usually does go through there.
> >
> > So I hadn't even really considered it.
> 
> OK... it just causes Andrew headaches I suspect. But if he prefers
> to hold onto it for an entire release cycle... Andrew?
> 

Yup.  A mechanical rename-foo-to-bar can be prepared and merged late -
there's little payback for the pain of maintaining it for a couple of
months.

Would prefer either that we hold off until after 2.6.27 is released or
just do it now.

> 
> > I don't mind the patch per se, but can you give some background on what
> > the pending optimization is that makes such a big difference?
> 
> Using the lock semantics bitops is the first one. While it is true
> that we could just hack them into TestSetPageLocked, I really prefer
> callers to require at least a cursory glance to convert them, and
> understand that this is a lock lock, and not a test_and_set bitop
> with full barrier semantics.

This patch wouldn't do anything to ensure that callers get the review
which you describe, would it?

(and we don't need to patch the code to just read it!)

> lock semantics bitops obviously doesn't help x86 a bit. It really
> helps powerpc though.
> 
> The part that helps x86 is another patch further down my stack, which
> can avoid looking at the hashed page waitqueue at unlock_page-time.
> Basically this eliminates the entire page waitqueue from the cache
> footprint of fastpath workloads where the lock is uncontended (often:
> page faulting, write(2), writeout etc).
> 
> I'll send all that stuff off to -mm for 2.6.28 if this gets upstream
> (or into -mm).

I'm OK with that.

Re: Switching TestSetPageLocked to trylock_page

[Benjamin Herrenschmidt]

> Yup.  A mechanical rename-foo-to-bar can be prepared and merged late -
> there's little payback for the pain of maintaining it for a couple of
> months.
> 
> Would prefer either that we hold off until after 2.6.27 is released or
> just do it now.

I'd like to have the performance improvement on powerpc in 2.6.27 :-)

I've always hated the way we abused bitops as locks, it's terribly
inefficient for us, so that's a nice cleanup as far as I'm concerned.

Cheers,
Ben.

Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin]

On Tuesday 05 August 2008 14:01, Benjamin Herrenschmidt wrote:
> > Yup.  A mechanical rename-foo-to-bar can be prepared and merged late -
> > there's little payback for the pain of maintaining it for a couple of
> > months.
> >
> > Would prefer either that we hold off until after 2.6.27 is released or
> > just do it now.
>
> I'd like to have the performance improvement on powerpc in 2.6.27 :-)

Well I would too, but I won't push the actual performance improvements
this time (maybe the lockdep work, though, if it is working).

I will try to get the rename in, and then get the performance
improvements into -mm and let Andrew merge when he's ready. To be honest,
I'm satisfied that 2.6.27 contains enough potential for npiggin to have
spectacularly broken the kernel...


> I've always hated the way we abused bitops as locks, it's terribly
> inefficient for us, so that's a nice cleanup as far as I'm concerned.

I agree, especially for widely used locks like page and buffer lock.
If *nothing* else it is much more readable...

Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin]

On Tuesday 05 August 2008 13:57, Andrew Morton wrote:
> On Tue, 5 Aug 2008 13:41:53 +1000 Nick Piggin <nickpiggin@yahoo.com.au> 
wrote:
> > On Tuesday 05 August 2008 13:28, Linus Torvalds wrote:
> > > On Tue, 5 Aug 2008, Nick Piggin wrote:
> > > > On Thursday 31 July 2008 17:26, Nick Piggin wrote:
> > > > > Hi,
> > > > >
> > > > > I'm wondering if I could get a patch merged which changes all
> > > > > TestSetPageLocked and replaces them with trylock_page?
> > > >
> > > > Yes? No?
> > > >
> > > > The alternative is try to merge it via -mm or -next, but that just
> > > > wastes everybodies time with conflicts of having these differences
> > > > between -mm and mainline.
> > >
> > > Heh. I had just been _assuming_ this would go through -mm, since it's
> > > exactly the kind of thing that usually does go through there.
> > >
> > > So I hadn't even really considered it.
> >
> > OK... it just causes Andrew headaches I suspect. But if he prefers
> > to hold onto it for an entire release cycle... Andrew?
>
> Yup.  A mechanical rename-foo-to-bar can be prepared and merged late -
> there's little payback for the pain of maintaining it for a couple of
> months.
>
> Would prefer either that we hold off until after 2.6.27 is released or
> just do it now.
>
> > > I don't mind the patch per se, but can you give some background on what
> > > the pending optimization is that makes such a big difference?
> >
> > Using the lock semantics bitops is the first one. While it is true
> > that we could just hack them into TestSetPageLocked, I really prefer
> > callers to require at least a cursory glance to convert them, and
> > understand that this is a lock lock, and not a test_and_set bitop
> > with full barrier semantics.
>
> This patch wouldn't do anything to ensure that callers get the review
> which you describe, would it?
>
> (and we don't need to patch the code to just read it!)

OK, not it doesn't really ensure that I guess. But it at least helps me
attempt to review callsites and maintainers of external patches hopefully
will get at least some heads up.

Basically it is the best we can do I think. I haven't seen any problems
in this area and I don't really expect to (only in some areas does the
lock implementation itself assume knowledge of barriers beyond regular
lock semantics).

In case something assumes a store won't be passed by TestSetPageLocked,
for example, it should be commented. Code that makes subtle use of
memory ordering and is not commented I think we can say from experience
is almost broken by definition :)

... no, that's not my excuse for introducing breakage.. I have *attempted*
to review callers, but it's not easy to detect uncommented use of barrier.

Re: Switching TestSetPageLocked to trylock_page

[Linus Torvalds]

On Mon, 4 Aug 2008, Andrew Morton wrote:
> 
> Would prefer either that we hold off until after 2.6.27 is released or
> just do it now.

Ok, I took that as an ack and just applied it.

		Linus

Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin]

On Tuesday 05 August 2008 14:32, Linus Torvalds wrote:
> On Mon, 4 Aug 2008, Andrew Morton wrote:
> > Would prefer either that we hold off until after 2.6.27 is released or
> > just do it now.
>
> Ok, I took that as an ack and just applied it.

Buffer trylock too? Pretty please ;)

Re: Switching TestSetPageLocked to trylock_page

[Linus Torvalds]

On Tue, 5 Aug 2008, Nick Piggin wrote:
> 
> Buffer trylock too? Pretty please ;)

Now you're just pushing your luck.

But yeah, I took that one too.

		Linus

Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin]

On Tuesday 05 August 2008 14:57, Linus Torvalds wrote:
> On Tue, 5 Aug 2008, Nick Piggin wrote:
> > Buffer trylock too? Pretty please ;)
>
> Now you're just pushing your luck.
>
> But yeah, I took that one too.

You're a gem. Thanks!

Re: Switching TestSetPageLocked to trylock_page

[Linus Torvalds]

On Tue, 5 Aug 2008, Nick Piggin wrote:
> 
> The part that helps x86 is another patch further down my stack, which
> can avoid looking at the hashed page waitqueue at unlock_page-time.
> Basically this eliminates the entire page waitqueue from the cache
> footprint of fastpath workloads where the lock is uncontended (often:
> page faulting, write(2), writeout etc).

Hmm. Tell me more. Contention bit?

		Linus

Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin]

On Tuesday 05 August 2008 14:25, Linus Torvalds wrote:
> On Tue, 5 Aug 2008, Nick Piggin wrote:
> > The part that helps x86 is another patch further down my stack, which
> > can avoid looking at the hashed page waitqueue at unlock_page-time.
> > Basically this eliminates the entire page waitqueue from the cache
> > footprint of fastpath workloads where the lock is uncontended (often:
> > page faulting, write(2), writeout etc).
>
> Hmm. Tell me more. Contention bit?

Exactly. This is around the latest one online from a quick google:

http://www.kernel.org/pub/linux/kernel/people/npiggin/patches/lock-bitops/2.6.22-rc4/mm-unlock-speedup.patch