Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ad3fae63-984b-3a4e-4bfc-a09db0ad35b0@suse.cz>
Date:   Tue, 25 Oct 2022 16:08:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: amusing SLUB compaction bug when CC_OPTIMIZE_FOR_SIZE
Content-Language: en-US
To:     Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc:     Matthew Wilcox <willy@infradead.org>,
        Hugh Dickins <hughd@google.com>,
        David Laight <David.Laight@aculab.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        rcu@vger.kernel.org, "Paul E . McKenney" <paulmck@kernel.org>
References: <ea96c78c-e1dc-1364-e91-51909f82388b@google.com>
 <YzPgTtFzpKEfwPbK@hyeyoo> <YzRQvoVsnJzsauwb@google.com>
 <35502bdd-1a78-dea1-6ac3-6ff1bcc073fa@suse.cz>
 <ff905c1e-5eb3-eaf8-46de-38f189c0b7a5@google.com>
 <de71b83a-c82c-4785-ef5a-3db4f17bbc8d@suse.cz>
 <bcecece-f7ce-221d-1674-da3d5ab3fef@google.com> <YzkmErHFyYW3awRn@hyeyoo>
 <YzsVM8eToHUeTP75@casper.infradead.org>
 <7dddca4c-bc36-2cf0-de1c-a770bef9e1b7@suse.cz> <Y1fpABCR3/Vs/u0H@hyeyoo>
From:   Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <Y1fpABCR3/Vs/u0H@hyeyoo>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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