Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] mm/slab: protect cache_reap() against CPU and memory hot
 plug operations
To:     Michal Hocko <mhocko@kernel.org>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        stable@vger.kernel.org, Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>
References: <20190311191701.24325-1-ldufour@linux.ibm.com>
 <20190312145813.GS5721@dhcp22.suse.cz>
From:   Laurent Dufour <ldufour@linux.ibm.com>
Date:   Tue, 12 Mar 2019 17:28:47 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0)
 Gecko/20100101 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <20190312145813.GS5721@dhcp22.suse.cz>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Message-Id: <b2b80faf-2670-da69-60d9-f244d1597139@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Le 12/03/2019 à 15:58, Michal Hocko a écrit :
> On Mon 11-03-19 20:17:01, Laurent Dufour wrote:
>> The commit 95402b382901 ("cpu-hotplug: replace per-subsystem mutexes with
>> get_online_cpus()") remove the CPU_LOCK_ACQUIRE operation which was use to
>> grap the cache_chain_mutex lock which was protecting cache_reap() against
>> CPU hot plug operations.
>>
>> Later the commit 18004c5d4084 ("mm, sl[aou]b: Use a common mutex
>> definition") changed cache_chain_mutex to slab_mutex but this didn't help
>> fixing the missing the cache_reap() protection against CPU hot plug
>> operations.
>>
>> Here we are stopping the per cpu worker while holding the slab_mutex to
>> ensure that cache_reap() is not running in our back and will not be
>> triggered anymore for this cpu.
>>
>> This patch fixes that race leading to SLAB's data corruption when CPU
>> hotplug are triggered. We hit it while doing partition migration on PowerVM
>> leading to CPU reconfiguration through the CPU hotplug mechanism.
> 
> What is the actual race? slab_offline_cpu calls cancel_delayed_work_sync
> so it removes a pending item and waits for the item to finish if they run
> concurently. So why do we need an additional lock?

You're right.
Reading cancel_delayed_work_sync() again I can't see how this could help.

The tests done with that patch were successful, while we were seeing a 
SLAB data corruption without it, but this needs to be investigated 
further since this one should not help. This was perhaps a lucky side 
effect.

Please forgot about this one.

> 
>> This fix is covering kernel containing to the commit 6731d4f12315 ("slab:
>> Convert to hotplug state machine"), ie 4.9.1, earlier kernel needs a
>> slightly different patch.
>>
>> Cc: stable@vger.kernel.org
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: Pekka Enberg <penberg@kernel.org>
>> Cc: David Rientjes <rientjes@google.com>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>   mm/slab.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index 28652e4218e0..ba499d90f27f 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -1103,6 +1103,7 @@ static int slab_online_cpu(unsigned int cpu)
>>   
>>   static int slab_offline_cpu(unsigned int cpu)
>>   {
>> +	mutex_lock(&slab_mutex);
>>   	/*
>>   	 * Shutdown cache reaper. Note that the slab_mutex is held so
>>   	 * that if cache_reap() is invoked it cannot do anything
>> @@ -1112,6 +1113,7 @@ static int slab_offline_cpu(unsigned int cpu)
>>   	cancel_delayed_work_sync(&per_cpu(slab_reap_work, cpu));
>>   	/* Now the cache_reaper is guaranteed to be not running. */
>>   	per_cpu(slab_reap_work, cpu).work.func = NULL;
>> +	mutex_unlock(&slab_mutex);
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.21.0
>