Re: [PATCH v3] mm,oom: speed up select_bad_process() loop.

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

Michal Hocko wrote:
> On Fri 20-05-16 20:51:56, Tetsuo Handa wrote:
> [...]
> > +static bool has_pending_victim(struct task_struct *p)
> > +{
> > +	struct task_struct *t;
> > +	bool ret = false;
> > +
> > +	rcu_read_lock();
> > +	for_each_thread(p, t) {
> > +		if (test_tsk_thread_flag(t, TIF_MEMDIE)) {
> > +			ret = true;
> > +			break;
> > +		}
> > +	}
> > +	rcu_read_unlock();
> > +	return ret;
> > +}
> 
> And so you do not speed up anything in the end because you have to
> iterate all threads anyway yet you add quite some code on top. No I do
> not like it. This is no longer a cleanup...

I changed for_each_process_thread() to for_each_process(). This means
O(num_threads^2) task_in_mem_cgroup() and O(num_threads^2)
has_intersects_mems_allowed() are replaced with O(num_threads)
task_in_mem_cgroup() and O(num_threads) has_intersects_mems_allowed()
at the cost of adding O(num_threads) has_pending_victim().

I expect that O(num_threads) (task_in_mem_cgroup() + has_intersects_mems_allowed() +
has_pending_victim()) is faster than O(num_threads^2) (task_in_mem_cgroup() +
has_intersects_mems_allowed()) + O(num_threads) test_tsk_thread_flag().

> 
> [...]
> > Note that "[PATCH v3] mm,oom: speed up select_bad_process() loop." temporarily
> > broke oom_task_origin(task) case, for oom_select_bad_process() might select
> > a task without mm because oom_badness() which checks for mm != NULL will not be
> > called.
> 
> How can we have oom_task_origin without mm? The flag is set explicitly
> while doing swapoff resp. writing to ksm. We clear the flag before
> exiting.

What if oom_task_origin(task) received SIGKILL, but task was unable to run for
very long period (e.g. 30 seconds) due to scheduling priority, and the OOM-reaper
reaped task's mm within a second. Next round of OOM-killer selects the same task
due to oom_task_origin(task) without doing MMF_OOM_REAPED test.

Once the OOM-reaper reaped task's mm (or gave up reaping it), subsequent
OOM-killer should treat that task as task->mm = NULL. Moving
oom_task_origin(task) test to after test_bit(MMF_OOM_REAPED, &p->mm->flags)
test will let the OOM-killer think as "oom_task_origin without mm".

> 
> [...]
> 
> > By the way, I noticed that mem_cgroup_out_of_memory() might have a bug about its
> > return value. It returns true if hit OOM_SCAN_ABORT after chosen != NULL, false
> > if hit OOM_SCAN_ABORT before chosen != NULL. Which is expected return value?
> 
> true. Care to send a patch?

I don't know what memory_max_write() wants to do when it found a TIF_MEMDIE thread
in the given memcg. Thus, I can't tell whether setting chosen to NULL (which means
mem_cgroup_out_of_memory() returns false) is the expected behavior.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>