hi Matthew, Thanks a lot for comments! 在 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. > return lruvec; > > 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? Thanks a lot! Alex