On Thu, Mar 25, 2021 at 09:04:40PM -0700, Hugh Dickins wrote:
> On Fri, 26 Mar 2021, Matthew Wilcox wrote:
> > On Thu, Mar 25, 2021 at 06:55:42PM -0700, Hugh Dickins wrote:
> > > The first reason occurred to me this morning. I thought I had been
> > > clever to spot the PageHead race which you fix here. But now I just feel
> > > very stupid not to have spotted the very similar memcg_data race. The
> > > speculative racer may call mem_cgroup_uncharge() from __put_single_page(),
> > > and the new call to split_page_memcg() do nothing because page_memcg(head)
> > > is already NULL.
> > >
> > > And is it even safe there, to sprinkle memcg_data through all of those
> > > order-0 subpages, when free_the_page() is about to be applied to a
> > > series of descending orders? I could easily be wrong, but I think
> > > free_pages_prepare()'s check_free_page() will find that is not
> > > page_expected_state().
I forgot to say earlier; I did add a test (lib/test_free_pages.c).
Doubling it up to check GFP_KERNEL | GFP_ACCOUNT and GFP_KERNEL |
GFP_COMP | GFP_ACCOUNT should be reasonable.
> > So back to something more like my original patch then?
> >
> > +++ b/mm/page_alloc.c
> > @@ -5081,9 +5081,15 @@ void __free_pages(struct page *page, unsigned int order)
> > {
> > if (put_page_testzero(page))
> > free_the_page(page, order);
> > - else if (!PageHead(page))
> > - while (order-- > 0)
> > - free_the_page(page + (1 << order), order);
> > + else if (!PageHead(page)) {
> > + while (order-- > 0) {
> > + struct page *tail = page + (1 << order);
> > +#ifdef CONFIG_MEMCG
> > + tail->memcg_data = page->memcg_data;
> > +#endif
> > + free_the_page(tail, order);
> > + }
> > + }
> > }
> > EXPORT_SYMBOL(__free_pages);
> >
> > We can cache page->memcg_data before calling put_page_testzero(),
> > just like we cache the Head flag in Johannes' patch.
>
> If I still believed in e320d3012d25, yes, that would look right
> (but I don't have much faith in my judgement after all this).
>
> I'd fallen in love with split_page_memcg() when you posted that
> one, and was put off by your #ifdef, so got my priorities wrong
> and went for the split_page_memcg().
Oh, the ifdef was just a strawman. I wouldn't want to see that upstream;
something like:
unsigned long memcg_data = __get_memcg_data(page);
...
__set_memcg_data(tail, memcg_data);
with the appropriate ifdefs hidden in memcontrol.h would be my preference.
> > > But, after all that, I'm now thinking that Matthew's original
> > > e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages")
> > > is safer reverted. The put_page_testzero() in __free_pages() was
> > > not introduced for speculative pagecache: it was there in 2.4.0,
> > > and atomic_dec_and_test() in 2.2, I don't have older trees to hand.
> >
> > I think you're confused in that last assertion. According to
> > linux-fullhistory, the first introduction of __free_pages was 2.3.29pre3
> > (September 1999), where it did indeed use put_page_testzero:
>
> Not confused, just pontificating from a misleading subset of the data.
> I knew there's an even-more-history-than-tglx git tree somewhere, but
> what I usually look back to is 2.4 trees, plus a 2.2.26 tree - but of
> course that's a late 2.2, from 2004, around the same time as 2.6.3.
I suspect it got backported ...
https://github.com/mpe/linux-fullhistory/wiki is what I'm using for my
archaeology, and it doesn't have the stable branches (1.0, 1.2, 2.0,
2.2, 2.4), so I don't know for sure.
Anyway, my point is that the truly ancient drivers *don't* depend on this
behaviour because the function didn't even exist when they were written.
