Message-ID: <Zl4wY68a5QI8Ph6w@x1n>
Date: Mon, 3 Jun 2024 17:06:43 -0400
From: Peter Xu <peterx@...hat.com>
To: David Hildenbrand <david@...hat.com>,
	Nicholas Piggin <npiggin@...il.com>
Cc: 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

[Copy Nicolas for real]

On Mon, Jun 03, 2024 at 05:03:16PM -0400, 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,
> 
> -- 
> Peter Xu

-- 
Peter Xu

Powered by blists - more mailing lists