On Thu, Mar 25, 2021 at 09:04:40PM -0700, Hugh Dickins wrote:

On Fri, 26 Mar 2021, Matthew Wilcox wrote:

quoted

On Thu, Mar 25, 2021 at 06:55:42PM -0700, Hugh Dickins wrote:

quoted

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.

quoted

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.

quoted

quoted

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.