Re: [PATCH 05/12] mm: trylock_page

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

On Sunday 30 September 2007 01:01, Peter Zijlstra wrote:
> On Fri, 2007-09-28 at 13:11 +1000, Nick Piggin wrote:
> > On Friday 28 September 2007 17:42, Peter Zijlstra wrote:
> > > Replace raw TestSetPageLocked() usage with trylock_page()
> >
> > I have such a thing queued too, for the lock bitops patches for when
> > 2.6.24 opens, Andrew promises me :).
> >
> > I guess they should be identical, except I don't like doing trylock_page
> > in place of SetPageLocked, for memory ordering performance and aesthetic
> > reasons... I've got an init_page_locked (or set_page_locked... I can't
> > remember, the patch is at home).
>
> Sure, that might work, or we could just make it so that add_to_*_cache
> is never passed an unlocked page. But sure...

It does kind of make sense to have it there (because you want the page
to be locked iff it gets inserted into pagecache). And wherever you lock
the page, we'll still want an init_page_locked to be able to lock it while we
are the only owner of it, for the same performance reason.


> > Fine idea to lockdep the page lock, anyway. Does it show up any of the
> > buffered write deadlock possibilities? :)
>
> Not yet, it might just be that the concessions done to annotate this
> type of lock were too severe.
>
> What I basically did was treat all the page locks as a single recursive
> lock.

Hmm, OK. There are a couple of page lock deadlocks there that wouldn't
be picked up, but the page lock vs mmap_sem one probably should be.


> > buffer lock is another notable bit-mutex that might be converted (I have
> > the patch to do the similar nice !tas->trylock conversion for that too).
> > I think it is used widely enough by tricky code that it would be useful
> > to annotate as well.
>
> Not at all familiar with that lock, but yeah, we could have a look at
> doing that too.

Should be pretty well identical to the page lock. I'll cc you on the patch
to do this equivalent API conversion, if you'd like.


> > Unfortunately we can't convert bit_spinlock.h easily, I guess?
>
> Yeah, the space constraints make that rather hard. Each of these locks
> needs some form of external meta-data.

Yeah.

> For the page lock I used one lock instance per file system type.

Seems OK.