Re: [PATCH v2 4/8] mm/lru: only change the lru_lock iff page's lruvec is different

[Matthew Wilcox]

On Wed, Nov 13, 2019 at 10:26:24AM +0800, Alex Shi wrote:
> å 2019/11/12 äå10:36, Matthew Wilcox åé:
> > On Tue, Nov 12, 2019 at 10:06:24PM +0800, Alex Shi wrote:
> >> +/* Don't lock again iff page's lruvec locked */
> >> +static inline struct lruvec *relock_page_lruvec_irq(struct page *page,
> >> +					struct lruvec *locked_lruvec)
> >> +{
> >> +	struct pglist_data *pgdat = page_pgdat(page);
> >> +	struct lruvec *lruvec;
> >> +
> >> +	rcu_read_lock();
> >> +	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> >> +
> >> +	if (locked_lruvec == lruvec) {
> >> +		rcu_read_unlock();
> >> +		return lruvec;
> >> +	}
> >> +	rcu_read_unlock();
> > 
> > Why not simply:
> > 
> > 	rcu_read_lock();
> > 	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> > 	rcu_read_unlock();
> > 
> > 	if (locked_lruvec == lruvec)
> 
> The rcu_read_unlock here is for guarding the locked_lruvec/lruvec comparsion.
> Otherwise memcg/lruvec maybe changed, like, from memcg migration etc. I guess.

How does holding the RCU lock guard the comparison?  You're comparing two
pointers for equality.  Nothing any other CPU can do at this point will
change the results of that comparison.

> > Also, why are you bothering to re-enable interrupts here?  Surely if
> > you're holding lock A with interrupts disabled , you can just drop lock A,
> > acquire lock B and leave the interrupts alone.  That way you only need
> > one of this variety of function, and not the separate irq/irqsave variants.
> > 
> 
> Thanks for the suggestion! Yes, if only do re-lock, it's better to leave the irq unchanging. but, when the locked_lruvec is NULL, it become a first time lock which irq or irqsave are different. Thus, in combined function we need a nother parameter to indicate if it do irqsaving. So comparing to a extra/indistinct parameter, I guess 2 inline functions would be a bit more simple/cleary? 

Ah, right, I missed the "if it's not held" case.