Re: [mm] 4e2c82a409: ltp.overcommit_memory01.fail

[Feng Tang <feng.tang@intel.com>]

On Tue, Jul 07, 2020 at 01:41:20PM +0800, Feng Tang wrote:
> On Tue, Jul 07, 2020 at 12:00:09PM +0800, Huang, Ying wrote:
> > Feng Tang <feng.tang@intel.com> writes:
> > 
> > > On Mon, Jul 06, 2020 at 06:34:34AM -0700, Andi Kleen wrote:
> > >> >  	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > >> > -	if (ret == 0 && write)
> > >> > +	if (ret == 0 && write) {
> > >> > +		if (sysctl_overcommit_memory == OVERCOMMIT_NEVER)
> > >> > +			schedule_on_each_cpu(sync_overcommit_as);
> > >> 
> > >> The schedule_on_each_cpu is not atomic, so the problem could still happen
> > >> in that window.
> > >> 
> > >> I think it may be ok if it eventually resolves, but certainly needs
> > >> a comment explaining it. Can you do some stress testing toggling the
> > >> policy all the time on different CPUs and running the test on
> > >> other CPUs and see if the test fails?
> > >
> > > For the raw test case reported by 0day, this patch passed in 200 times
> > > run. And I will read the ltp code and try stress testing it as you
> > > suggested.
> > >
> > >
> > >> The other alternative would be to define some intermediate state
> > >> for the sysctl variable and only switch to never once the schedule_on_each_cpu
> > >> returned. But that's more complexity.
> > >
> > > One thought I had is to put this schedule_on_each_cpu() before
> > > the proc_dointvec_minmax() to do the sync before sysctl_overcommit_memory
> > > is really changed. But the window still exists, as the batch is
> > > still the larger one. 
> > 
> > Can we change the batch firstly, then sync the global counter, finally
> > change the overcommit policy?
> 
> These reorderings are really head scratching :)
> 
> I've thought about this before when Qian Cai first reported the warning
> message, as kernel had a check: 
> 
> 	VM_WARN_ONCE(percpu_counter_read(&vm_committed_as) <
> 			-(s64)vm_committed_as_batch * num_online_cpus(),
> 			"memory commitment underflow");
> 
> If the batch is decreased first, the warning will be easier/earlier to be
> triggered, so I didn't brought this up when handling the warning message.
> 
> But it might work now, as the warning has been removed.

I tested the reorder way, and the test could pass in 100 times run. The
new order when changing policy to OVERCOMMIT_NEVER:
  1. re-compute the batch ( to the smaller one)
  2. do the on_each_cpu sync
  3. really change the policy to NEVER.

It solves one of previous concern, that after the sync is done on cpuX,
but before the whole sync on all cpus are done, there is a window that
the percpu-counter could be enlarged again.

IIRC Andi had concern about read side cost when doing the sync, my
understanding is most of the readers (malloc/free/map/unmap) are using
percpu_counter_read_positive, which is a fast path without involving lock.

As for the problem itself, I agree with Michal's point, that usually there
is no normal case that will change the overcommit_policy too frequently.

The code logic is mainly in overcommit_policy_handler(), based on the
previous sync fix. please help to review, thanks!

int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
		size_t *lenp, loff_t *ppos)
{
	int ret;

	if (write) {
		int new_policy;
		struct ctl_table t;

		t = *table;
		t.data = &new_policy;
		ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
		if (ret)
			return ret;

		mm_compute_batch(new_policy);
		if (new_policy == OVERCOMMIT_NEVER)
			schedule_on_each_cpu(sync_overcommit_as);
		sysctl_overcommit_memory = new_policy;
	} else {
		ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
	}

	return ret;
}

- Feng