Re: amusing SLUB compaction bug when CC_OPTIMIZE_FOR_SIZE

Vlastimil Babka
2022-10-25T14:10:43Z

On 10/25/22 15:47, Hyeonggon Yoo wrote:
> On Mon, Oct 24, 2022 at 04:35:04PM +0200, Vlastimil Babka wrote:
>
> [,,,]
>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index 59c8e28f7b6a..219beb48588e 100644
>> --- a/mm/slab.c
>> +++ 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();
>> /* Record if ALLOC_NO_WATERMARKS was set when allocating the slab */
>> if (sk_memalloc_socks() && page_is_pfmemalloc(folio_page(folio, 0)))
>> slab_set_pfmemalloc(slab);
>> @@ -1387,9 +1389,11 @@ static void kmem_freepages(struct kmem_cache *cachep, struct slab *slab)
>>
>> BUG_ON(!folio_test_slab(folio));
>> __slab_clear_pfmemalloc(slab);
>> - __folio_clear_slab(folio);
>> page_mapcount_reset(folio_page(folio, 0));
>> folio->mapping = NULL;
>> + /* Make the mapping reset visible before clearing the flag */
>> + smp_wmb();
>> + __folio_clear_slab(folio);
>>
>> if (current->reclaim_state)
>> current->reclaim_state->reclaimed_slab += 1 << order;
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 157527d7101b..6dc17cb915c5 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1800,6 +1800,8 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node,
>>
>> slab = folio_slab(folio);
>> __folio_set_slab(folio);
>> + /* Make the flag visible before any changes to folio->mapping */
>> + smp_wmb();
>> if (page_is_pfmemalloc(folio_page(folio, 0)))
>> slab_set_pfmemalloc(slab);
>>
>> @@ -2008,8 +2010,10 @@ static void __free_slab(struct kmem_cache *s, struct slab *slab)
>> }
>>
>> __slab_clear_pfmemalloc(slab);
>> - __folio_clear_slab(folio);
>> folio->mapping = NULL;
>> + /* Make the mapping reset visible before clearing the flag */
>> + smp_wmb();
>> + __folio_clear_slab(folio);
>> if (current->reclaim_state)
>> current->reclaim_state->reclaimed_slab += pages;
>> unaccount_slab(slab, order, s);
>> --
>> 2.38.0
>
> Do we need to try this with memory barriers before frozen refcount lands in?

There was IIRC an unresolved issue with frozen refcount tripping the page
isolation code so I didn't want to be depending on that.

> It's quite complicated and IIUC there is a still theoretical race:
>
> At isolate_movable_page: At slab alloc: At slab free:
> folio = alloc_pages(flags, order)
>
> folio_try_get()
> folio_test_slab() == false
> __folio_set_slab(folio)
> smp_wmb()
>
> call_rcu(&slab->rcu_head, rcu_free_slab);
>
>
> smp_rmb()
> __folio_test_movable() == true
>
> folio->mapping = NULL;
> smp_wmb()
> __folio_clear_slab(folio);
> smp_rmb()
> folio_test_slab() == false
>
> folio_trylock()

There's also between above and below:

if (!PageMovable(page) || PageIsolated(page))
goto out_no_isolated;

mops = page_movable_ops(page);

If we put another smp_rmb() before the PageMovable test, could that have
helped? It would assure we observe the folio->mapping = NULL; from the "slab
free" side?

But yeah, it's getting ridiculous. Maybe there's a simpler way to check two
bits in two different bytes atomically. Or maybe it's just an impossible
task, I feel I just dunno computers at this point.

> mops->isolate_page() (*crash*)
>
>
> Please let me know if I'm missing something ;-)
> Thanks!
>