Re: [PATCH] mm: filemap: Avoid unnecessary barries and waitqueue lookups in unlock_page fastpath v5

[Mel Gorman]

On Thu, May 22, 2014 at 10:47:22AM -0700, Andrew Morton wrote:
> On Thu, 22 May 2014 09:46:43 +0100 Mel Gorman <mgorman@xxxxxxx> wrote:
> 
> > > > If I'm still on track here, what happens if we switch to wake-all so we
> > > > can avoid the dangling flag?  I doubt if there are many collisions on
> > > > that hash table?
> > > 
> > > Wake-all will be ugly and loose a herd of waiters, all racing to
> > > acquire, all but one of whoem will loose the race. It also looses the
> > > fairness, its currently a FIFO queue. Wake-all will allow starvation.
> > > 
> > 
> > And the cost of the thundering herd of waiters may offset any benefit of
> > reducing the number of calls to page_waitqueue and waker functions.
> 
> Well, none of this has been demonstrated.
> 

True, but it's also the type of thing that would deserve a patch of its
own with some separation in case bisection fingerpoints to a patch that
is doing too much on its own.

> As I speculated earlier, hash chain collisions will probably be rare,

They are meant to be (well, they're documented to be). It's the primary
reason why I'm not concerned about "dangling waiters" being that common
a case.

> except for the case where a bunch of processes are waiting on the same
> page.  And in this case, perhaps wake-all is the desired behavior.
> 
> Take a look at do_read_cache_page().  It does lock_page(), but it
> doesn't actually *need* to.  It checks ->mapping and PG_uptodate and
> then...  unlocks the page!  We could have used wait_on_page_locked()
> there and permitted concurrent threads to run concurrently.
> 

It does that later when it calls wait_on_page_read but the flow is weird. It
looks like the first lock_page was to serialise against any IO and double
check it was not racing against a parallel reclaim although the elevated
reference count should have prevented that. Historical artifact maybe?
It looks like there could be some improvement there but also would deserve
a patch on its own.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/