| Message ID | 1427264236-17249-4-git-send-email-hannes@cmpxchg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 25 Mar 2015, Johannes Weiner wrote: > exit_oom_victim() already knows that TIF_MEMDIE is set, and nobody > else can clear it concurrently. Use clear_thread_flag() directly. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> For the oom killer, that's true because of task_lock(): we always only set TIF_MEMDIE when there is a valid p->mm and it's cleared in the exit path after the unlock, acting as a barrier, when p->mm is set to NULL so it's no longer a valid victim. So that part is fine. The problem is the android low memory killer that does mark_tsk_oom_victim() without the protection of task_lock(), it's just rcu protected so the reference to the task itself is guaranteed to still be valid. I assume that's why Michal implemented it this way and added the comment to the lmk in commit 49550b605587 ("oom: add helpers for setting and clearing TIF_MEMDIE") to avoid TIF_MEMDIE entirely there. > --- > mm/oom_kill.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index b2f081fe4b1a..4b9547be9170 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -435,8 +435,7 @@ void mark_oom_victim(struct task_struct *tsk) > */ > void exit_oom_victim(void) > { > - if (!test_and_clear_thread_flag(TIF_MEMDIE)) > - return; > + clear_thread_flag(TIF_MEMDIE); > > down_read(&oom_sem); > /* -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, On Wed, Mar 25, 2015 at 08:31:49PM -0700, David Rientjes wrote: > On Wed, 25 Mar 2015, Johannes Weiner wrote: > > > exit_oom_victim() already knows that TIF_MEMDIE is set, and nobody > > else can clear it concurrently. Use clear_thread_flag() directly. > > > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > For the oom killer, that's true because of task_lock(): we always only set > TIF_MEMDIE when there is a valid p->mm and it's cleared in the exit path > after the unlock, acting as a barrier, when p->mm is set to NULL so it's > no longer a valid victim. So that part is fine. > > The problem is the android low memory killer that does > mark_tsk_oom_victim() without the protection of task_lock(), it's just rcu > protected so the reference to the task itself is guaranteed to still be > valid. But this is about *setting* it without a lock. My point was that once TIF_MEMDIE is actually set, the task owns it and nobody else can clear it for them, so it's safe to test and clear non-atomically from the task's own context. Am I missing something? -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 25-03-15 02:17:07, Johannes Weiner wrote: > exit_oom_victim() already knows that TIF_MEMDIE is set, and nobody > else can clear it concurrently. Use clear_thread_flag() directly. Yeah. This is a left over from the review process. I originally did unmarking unconditionally but Tejun suggested calling test_thread_flag before calling here. So the test_and_clear is safe here. > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> Acked-by: Michal Hocko <mhocko@suse.cz> > --- > mm/oom_kill.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index b2f081fe4b1a..4b9547be9170 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -435,8 +435,7 @@ void mark_oom_victim(struct task_struct *tsk) > */ > void exit_oom_victim(void) > { > - if (!test_and_clear_thread_flag(TIF_MEMDIE)) > - return; > + clear_thread_flag(TIF_MEMDIE); > > down_read(&oom_sem); > /* > -- > 2.3.3 >
diff --git a/mm/oom_kill.c b/mm/oom_kill.c index b2f081fe4b1a..4b9547be9170 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -435,8 +435,7 @@ void mark_oom_victim(struct task_struct *tsk) */ void exit_oom_victim(void) { - if (!test_and_clear_thread_flag(TIF_MEMDIE)) - return; + clear_thread_flag(TIF_MEMDIE); down_read(&oom_sem); /*
exit_oom_victim() already knows that TIF_MEMDIE is set, and nobody else can clear it concurrently. Use clear_thread_flag() directly. Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/oom_kill.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)