Date: Mon, 3 Jun 2024 15:43:43 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: David Hildenbrand <david@...hat.com>
Cc: Peter Xu <peterx@...hat.com>, Yang Shi <shy828301@...il.com>,
	kernel test robot <oliver.sang@...el.com>,
	Jason Gunthorpe <jgg@...dia.com>,
	Vivek Kasireddy <vivek.kasireddy@...el.com>,
	Rik van Riel <riel@...riel.com>, oe-lkp@...ts.linux.dev,
	lkp@...el.com, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Matthew Wilcox <willy@...radead.org>,
	Christopher Lameter <cl@...ux.com>, linux-mm@...ck.org
Subject: Re: [linus:master] [mm] efa7df3e3b:
 kernel_BUG_at_include/linux/page_ref.h

On Mon, Jun 03, 2024 at 11:05:44PM +0200, David Hildenbrand wrote:
> On 03.06.24 23:03, Peter Xu wrote:
> > On Mon, Jun 03, 2024 at 10:37:55PM +0200, David Hildenbrand wrote:
> > > > > try_get_folio() is all about grabbing a folio that might get freed
> > > > > concurrently. That's why it calls folio_ref_try_add_rcu() and does
> > > > > complicated stuff.
> > > > 
> > > > IMHO we can define it.. e.g. try_get_page() wasn't defined as so.
> > > > 
> > > > If we want to be crystal clear on that and if we think that's what we want,
> > > > again I would suggest we rename it differently from try_get_page() to avoid
> > > > future misuses, then add documents. We may want to also even assert the
> > > 
> > > Yes, absolutely.
> > > 
> > > > rcu/irq implications in try_get_folio() at entrance, then that'll be
> > > > detected even without TINY_RCU config.
> > > > 
> > > > > 
> > > > > On !CONFIG_TINY_RCU, it performs a folio_ref_add_unless(). That's
> > > > > essentially a atomic_add_unless(), which in the worst case ends up being a
> > > > > cmpxchg loop.
> > > > > 
> > > > > 
> > > > > Stating that we should be using try_get_folio() in paths where we are sure
> > > > > the folio refcount is not 0 is the same as using folio_try_get() where
> > > > > folio_get() would be sufficient.
> > > > > 
> > > > > The VM_BUG_ON in folio_ref_try_add_rcu() really tells us here that we are
> > > > > using a function in the wrong context, although in our case, it is safe to
> > > > > use (there is now BUG). Which is true, because we know we have a folio
> > > > > reference and can simply use a simple folio_ref_add().
> > > > > 
> > > > > Again, just like we have folio_get() and folio_try_get(), we should
> > > > > distinguish in GUP whether we are adding more reference to a folio (and
> > > > > effectively do what folio_get() would), or whether we are actually grabbing
> > > > > a folio that could be freed concurrently (what folio_try_get() would do).
> > > > 
> > > > Yes we can.  Again, IMHO it's a matter of whether it will worth it.
> > > > 
> > > > Note that even with SMP and even if we keep this code, the
> > > > atomic_add_unless only affects gup slow on THP only, and even with that
> > > > overhead it is much faster than before when that path was introduced.. and
> > > > per my previous experience we don't care too much there, really.
> > > > 
> > > > So it's literally only three paths that are relevant here on the "unless"
> > > > overhead:
> > > > 
> > > >     - gup slow on THP (I just added it; used to be even slower..)
> > > > 
> > > >     - vivik's new path
> > > > 
> > > >     - hugepd (which may be gone for good in a few months..)
> > > > IMHO none of them has perf concerns.  The real perf concern paths is
> > > > gup-fast when pgtable entry existed, but that must use atomic_add_unless()
> > > > anyway.  Even gup-slow !thp case won't regress as that uses try_get_page().
> > > 
> > > My point is primarily that we should be clear that the one thing is
> > > GUP-fast, and the other is for GUP-slow.
> > 
> > Yes, understood.
> > 
> > > 
> > > Sooner or later we'll see more code that uses try_grab_page() to be
> > > converted to folios, and people might naturally use try_grab_folio(), just
> > > like we did with Vivik's code.
> > > 
> > > And I don't think we'll want to make GUP-slow in general using
> > > try_grab_folio() in the future.
> > > 
> > > So ...
> > > 
> > > > 
> > > > So again, IMHO the easist way to fix this WARN is we drop the TINY_RCU bit,
> > > > if nobody worries on UP perf.
> > > > 
> > > > I don't have a strong opinion, if any of us really worry about above three
> > > > use cases on "unless" overhead, and think it worthwhile to add the code to
> > > > support it, I won't object. But to me it's adding pain with no real benefit
> > > > we could ever measure, and adding complexity to backport too since we'll
> > > > need a fix for as old as 6.6.
> > > 
> > > ... for the sake of fixing this WARN, I don't primarily care. Adjusting the
> > > TINY_RCU feels wrong because I suspect somebody had good reasons to do it
> > > like that, and it actually reported something valuable (using the wrong
> > > function for the job).
> > > 
> > > In any case, if we take the easy route to fix the WARN, I'll come back and
> > > clean the functions here up properly.
> > 
> > Definitely, then there can also be some measurements which will be even
> > better.  I mean, if the diff is minimal, we can be clearer on the path we
> > choose; while if it shows improvements we have more solid results than
> > predictions and discussions.
> > 
> > Yes I do worry about the UP change too, hence I sincerely was trying to
> > collect some feedback.  My current guess is the UP was still important in
> > 2008 when the code first wrote, and maybe it changed over the 16 years. I
> > just notice Nicolas wrote it; I know he's still active so I've added him
> > into the loop too.
> > 
> > Just for easier reference, the commit introduced the UP change is:
> > 
> > commit e286781d5f2e9c846e012a39653a166e9d31777d
> > Author: Nicholas Piggin <npiggin@...il.com>
> > Date:   Fri Jul 25 19:45:30 2008 -0700
> > 
> >      mm: speculative page references
> > 
> > +static inline int page_cache_get_speculative(struct page *page)
> > +{
> > +       VM_BUG_ON(in_interrupt());
> > +
> > +#if !defined(CONFIG_SMP) && defined(CONFIG_CLASSIC_RCU)
> > +# ifdef CONFIG_PREEMPT
> > +       VM_BUG_ON(!in_atomic());
> > +# endif
> > +       /*
> > +        * Preempt must be disabled here - we rely on rcu_read_lock doing
> > +        * this for us.
> > +        *
> > +        * Pagecache won't be truncated from interrupt context, so if we have
> > +        * found a page in the radix tree here, we have pinned its refcount by
> > +        * disabling preempt, and hence no need for the "speculative get" that
> > +        * SMP requires.
> > +        */
> > +       VM_BUG_ON(page_count(page) == 0);
> > +       atomic_inc(&page->_count);
> > +
> > +#else
> > +       if (unlikely(!get_page_unless_zero(page))) {
> > +               /*
> > +                * Either the page has been freed, or will be freed.
> > +                * In either case, retry here and the caller should
> > +                * do the right thing (see comments above).
> > +                */
> > +               return 0;
> > +       }
> > +#endif
> > +       VM_BUG_ON(PageTail(page));
> > +
> > +       return 1;
> > +}
> > 
> > Thanks,
> > 
> 
> I chased it further to:
> 
> commit 8375ad98cc1defc36adf4a77d9ea1e71db51a371
> Author: Paul E. McKenney <paulmck@...nel.org>
> Date:   Mon Apr 29 15:06:13 2013 -0700
> 
>     vm: adjust ifdef for TINY_RCU
>     There is an ifdef in page_cache_get_speculative() that checks for !SMP
>     and TREE_RCU, which has been an impossible combination since the advent
>     of TINY_RCU.  The ifdef enables a fastpath that is valid when preemption
>     is disabled by rcu_read_lock() in UP systems, which is the case when
>     TINY_RCU is enabled.  This commit therefore adjusts the ifdef to
>     generate the fastpath when TINY_RCU is enabled.
> 
> 
> Where Paul explicitly restored that fastpath for TINY_RCU instead of removing that code.
> 
> So maybe Paul can comment if that is still worth having. CCing him.

It is currently an atomic operation either way, though the folio_ref_add()
avoids full ordering, but that is immaterial on x86.  Some say that it is
in the noise on server-class ARMv8 as well, though they have also said
a great many things in the past.  But if that is true, the big benefit
of the TINY_RCU check is that folio_ref_try_add_rcu() is guaranted not
to fail in that case (single CPU with preemption disabled).  Except that
everyone has to check the return value anyway, right?

So the usual advice, unsatisfying though it might be, is to remove that
#ifdef and see if anyone notices.

After all, both 2013 and 2008 were quite some time ago.  ;-)

							Thanx, Paul

Powered by blists - more mailing lists