Re: Locking comment on shrink_caches()

Josh MacDonald
2001-09-25T20:41:33Z

Quoting Marcelo Tosatti (marcelo@conectiva.com.br):
>
>
> On Tue, 25 Sep 2001, David S. Miller wrote:
>
> > From: Marcelo Tosatti <marcelo@conectiva.com.br>
> > Date: Tue, 25 Sep 2001 14:49:40 -0300 (BRT)
> >
> > Do you really need to do this ?
> >
> > if (unlikely(!spin_trylock(&pagecache_lock))) {
> > /* we hold the page lock so the page cannot go away from under us */
> > spin_unlock(&pagemap_lru_lock);
> >
> > spin_lock(&pagecache_lock);
> > spin_lock(&pagemap_lru_lock);
> > }
> >
> > Have you actually seen bad hold times of pagecache_lock by
> > shrink_caches() ?
> >
> > Marcelo, this is needed because of the spin lock ordering rules.
> > The pagecache_lock must be obtained before the pagemap_lru_lock
> > or else deadlock is possible. The spin_trylock is an optimization.
>
> Not, it is not.
>
> We can simply lock the pagecachelock and the pagemap_lru_lock at the
> beginning of the cleaning function. page_launder() use to do that.

Since your main concern seems to be simplicity, the code can remain
the way it is and be far more readable with, e.g.,

/* Aquire lock1 while holding lock2--reverse order. */
#define spin_reverse_lock(lock1,lock2) \
if (unlikely(!spin_trylock(&lock1))) { \
spin_unlock(&lock2); \
spin_lock(&lock1); \
spin_lock(&lock2); \
}

You can't argue for simple in favor of increasing lock contention,
but you can keep it readable.

-josh