Re: Locking comment on shrink_caches()

[Josh MacDonald <jmacd@CS.Berkeley.EDU>]

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