Re: amusing SLUB compaction bug when CC_OPTIMIZE_FOR_SIZE

[Vlastimil Babka <vbabka@suse.cz>]

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!
>