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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>