Re: Switching TestSetPageLocked to trylock_page

[Nick Piggin <nickpiggin@yahoo.com.au>]

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.