On 28.12.2018 14:00, Tetsuo Handa wrote: > On 2018/12/28 19:22, Kirill Tkhai wrote: >>> @@ -1389,8 +1389,13 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask, >>> }; >>> bool ret; >>> >>> - mutex_lock(&oom_lock); >>> - ret = out_of_memory(&oc); >>> + if (mutex_lock_killable(&oom_lock)) >>> + return true; >>> + /* >>> + * A few threads which were not waiting at mutex_lock_killable() can >>> + * fail to bail out. Therefore, check again after holding oom_lock. >>> + */ >>> + ret = fatal_signal_pending(current) || out_of_memory(&oc); >> >> This fatal_signal_pending() check has a sense because of >> it's possible, a killed task is waking up slowly, and it >> returns from schedule(), when there are no more waiters >> for a lock. > > Thanks. Michal thinks that mutex_lock_killable() would be sufficient > ( https://lkml.kernel.org/r/20181107100810.GA27423@xxxxxxxxxxxxxx ) but > I can confirm that mutex_lock_killable() is not sufficient when I test > using a VM with 8 CPUs. Thus, I'd like to keep this fatal_signal_pending() > check. > >> >> Why not make this approach generic, and add a check into >> __mutex_lock_common() after schedule_preempt_disabled() >> instead of this? This will handle all the places like >> that at once. >> >> (The only adding a check is not enough for __mutex_lock_common(), >> since mutex code will require to wake next waiter also. So, >> you will need a couple of changes in mutex code). > > I think that we should not assume that everybody is ready for making > mutex_lock_killable() to return -EINTR if fatal_signal_pending() is > true, and that adding below version would be a safer choice. There is signal_pending_state() primitive, and this is the check, which should be used instead of fatal_signal_pending() in mutex code. Let's ask Peter :) Peter, what you think about the approach overall? I.e., changing __mutex_lock_common() by adding one more check of signal_pending_state() after schedule_preempt_disabled() (with respect to other mutex code, e.g., waking next waiter etc)? Kirill
