On 10/24/22 17:06, Matthew Wilcox wrote: > On Mon, Oct 24, 2022 at 04:35:04PM +0200, Vlastimil Babka wrote: >> I would like to have a working safe version in -next, even if we are able >> simplify it later thanks to frozen refcounts. I've made a formal patch of >> yours, but I'm still convinced the slab check needs to be more paranoid so >> it can't observe a false positive __folio_test_movable() while missing the >> folio_test_slab(), hence I added the barriers as in my previous attempt [1]. >> Does that work for you and can I add your S-o-b? > > Thanks for picking this back up. > >> +++ b/mm/slab.c >> @@ -1370,6 +1370,8 @@ static struct slab *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, >> >> account_slab(slab, cachep->gfporder, cachep, flags); >> __folio_set_slab(folio); >> + /* Make the flag visible before any changes to folio->mapping */ >> + smp_wmb(); > > So what's the point of using __folio_set_slab() only to call smp_wmb() > afterwards? If we call folio_set_slab() instead, don't all the other > barriers go away? (This is a genuine question; I am bad at this kind > of reasoning). Obviously it would still need a comment. AFAIU (which doesn't mean much, TBH :)) folio_set_slab() makes the setting of the flag protected against other flags set operations so our setting is not lost in a non-atomic RMW. But as we are the only one who can be setting any page/folio flag here (isolate_movable_page() for sure doesn't), we don't need it for that kind of atomicity for page/folio flags field. And, simply changing it to folio_set_slab() would not add the sufficient smp_wmb() semantics to order the flags write visibility against a later write to the struct slab field that overlaps page->mapping. Only some atomic operations have that implicit barrier, (per Documentation/memory-barriers.txt and Documentation/atomic_bitops.txt) and set_bit() is not one of those. So we'd still need a smp_mb__after_atomic() AFAIU and at that point, doing the above seems less obscure to me. (Of course if we had the reason to use folio_set_slab() for its own atomic guarantee, then smp_mb__after_atomic() instead of smp_wmb() would be better as on some architectures it would make the barrier no-op).
