Re: [PATCH -mm -V7] mm, swap: fix race between swapoff and some swap operations

["Huang\, Ying" <ying.huang@intel.com>]

Hi, Andrea,

Andrea Arcangeli <aarcange@redhat.com> writes:

> Hello everyone,
>
> On Mon, Feb 11, 2019 at 04:38:46PM +0800, Huang, Ying wrote:
>> @@ -2386,7 +2463,17 @@ static void enable_swap_info(struct swap_info_struct *p, int prio,
>>  	frontswap_init(p->type, frontswap_map);
>>  	spin_lock(&swap_lock);
>>  	spin_lock(&p->lock);
>> -	 _enable_swap_info(p, prio, swap_map, cluster_info);
>> +	setup_swap_info(p, prio, swap_map, cluster_info);
>> +	spin_unlock(&p->lock);
>> +	spin_unlock(&swap_lock);
>> +	/*
>> +	 * Guarantee swap_map, cluster_info, etc. fields are used
>> +	 * between get/put_swap_device() only if SWP_VALID bit is set
>> +	 */
>> +	stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
>
> Should cpu_online_mask be read while holding cpus_read_lock?
>
> 	cpus_read_lock();
> 	err = __stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
> 	cpus_read_unlock();

Thanks for pointing this out.  Because swap_onoff_stop() is just dumb
function, something as below should be sufficient.

stop_machine(swap_onoff_stop, NULL, NULL);

> I missed what the exact motivation was for the switch from
> rcu_read_lock()/syncrhonize_rcu() to preempt_disable()/stop_machine().
>
> It looks like the above stop_machine all it does is to reach a
> quiescent point, when you've RCU that already can reach the quiescent
> point without an explicit stop_machine.
>
> The reason both implementations are basically looking the same is that
> stop_machine dummy call of swap_onoff_stop() { /* noop */ } will only
> reach a quiescent point faster than RCU, but it's otherwise
> functionally identical to RCU, but it's extremely more expensive. If
> it wasn't functionally identical stop_machine() couldn't be used as a
> drop in replacement of synchronize_sched() in the previous patch.
>
> I don't see the point of worrying about the synchronize_rcu latency in
> swapoff when RCU is basically identical and not more complex.
>
> So to be clear, I'm not against stop_machine() but with stop_machine()
> method invoked in all CPUs, you can actually do more than RCU and you
> can remove real locking not just reach a quiescent point.
>
> With stop_machine() the code would need reshuffling around so that the
> actual p->swap_map = NULL happens inside stop_machine, not outside
> like with RCU.
>
> With RCU all code stays concurrent at all times, simply the race is
> controlled, as opposed with stop_machine() you can make fully
> serialize and run like in UP temporarily (vs all preempt_disable()
> section at least).
>
> For example nr_swapfiles could in theory become a constant under
> preempt_disable() with stop_machine() without having to take a
> swap_lock.
>
> swap_onoff_stop can be implemented like this:
>
> enum {
> 	FIRST_STOP_MACHINE_INIT,
> 	FIRST_STOP_MACHINE_START,
> 	FIRST_STOP_MACHINE_END,
> };
> static int first_stop_machine;
> static int swap_onoff_stop(void *data)
> {
> 	struct swap_stop_machine *swsm = (struct swap_stop_machine *)data;
> 	int first;
>
> 	first = cmpxchg(&first_stop_machine, FIRST_STOP_MACHINE_INIT,
> 			FIRST_STOP_MACHINE_START);
> 	if (first == FIRST_STOP_MACHINE_INIT) {
> 		swsm->p->swap_map = NULL;
> 		/* add more stuff here until swap_lock goes away */
> 		smp_wmb();
> 		WRITE_ONCE(first_stop_machine, FIRST_STOP_MACHINE_END);
> 	} else {
> 		do {
> 			cpu_relax();
> 		} while (READ_ONCE(first_stop_machine) !=
> 			 FIRST_STOP_MACHINE_END);
> 		smp_rmb();
> 	}
>
> 	return 0;
> }
>
> stop_machine invoked with a method like above, will guarantee while we
> set p->swap_map to NULL (and while we do nr_swapfiles++) nothing else
> can run, no even interrupts, so some lock may just disappear. Only NMI
> and SMI could possibly run concurrently with the swsm->p->swap_map =
> NULL operation.
>
> If we've to keep swap_onoff_stop() a dummy function run on all CPUs
> just to reach a quiescent point, then I don't see why
> the synchronize_rcu() (or synchronize_sched or synchronize_kernel or
> whatever it is called right now, but still RCU) solution isn't
> preferable.

We only need to keep swap_onoff_stop() a dummy function as above.  So
from functionality point of view, RCU works for us perfectly too.  Paul
pointed out that before too.

Before, we choose to use stop_machine() to reduce the overhead of hot
path (page fault handler) as much as possible.  But now, I found
rcu_read_lock_sched() is just a wrapper of preempt_disable().  So maybe
we can switch to RCU version now.

Best Regards,
Huang, Ying

> Thanks,
> Andrea