| Message ID | 20220510030014.3842475-3-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] selftests: vm: add process_mrelease tests | expand |
On Mon 09-05-22 20:00:14, Suren Baghdasaryan wrote: > With the last usage of MMF_OOM_VICTIM in exit_mmap gone, this flag is > now unused and can be removed. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> LGTM Acked-by: Michal Hocko <mhocko@suse.com> One question below [...] > diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h > index 4d9e3a656875..746f6cb07a20 100644 > --- a/include/linux/sched/coredump.h > +++ b/include/linux/sched/coredump.h > @@ -70,7 +70,6 @@ static inline int get_dumpable(struct mm_struct *mm) > #define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */ > #define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */ > #define MMF_DISABLE_THP 24 /* disable THP for all VMAs */ > -#define MMF_OOM_VICTIM 25 /* mm is the oom victim */ > #define MMF_OOM_REAP_QUEUED 26 /* mm was queued for oom_reaper */ > #define MMF_MULTIPROCESS 27 /* mm is shared between processes */ Have you consider renumbering the follow up flags so that we do not have holes in there. Nothing really important but it can confuse somebody in the future.
On 5/9/22 9:00 PM, Suren Baghdasaryan wrote: > With the last usage of MMF_OOM_VICTIM in exit_mmap gone, this flag is > now unused and can be removed. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > include/linux/oom.h | 9 --------- > include/linux/sched/coredump.h | 1 - > mm/oom_kill.c | 4 +--- > 3 files changed, 1 insertion(+), 13 deletions(-) > > diff --git a/include/linux/oom.h b/include/linux/oom.h > index 6cdf0772dbae..25990e9d9e15 100644 > --- a/include/linux/oom.h > +++ b/include/linux/oom.h > @@ -77,15 +77,6 @@ static inline bool tsk_is_oom_victim(struct task_struct * tsk) > return tsk->signal->oom_mm; > } > > -/* > - * Use this helper if tsk->mm != mm and the victim mm needs a special > - * handling. This is guaranteed to stay true after once set. > - */ > -static inline bool mm_is_oom_victim(struct mm_struct *mm) > -{ > - return test_bit(MMF_OOM_VICTIM, &mm->flags); > -} > - > /* > * Checks whether a page fault on the given mm is still reliable. > * This is no longer true if the oom reaper started to reap the > diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h > index 4d9e3a656875..746f6cb07a20 100644 > --- a/include/linux/sched/coredump.h > +++ b/include/linux/sched/coredump.h > @@ -70,7 +70,6 @@ static inline int get_dumpable(struct mm_struct *mm) > #define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */ > #define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */ > #define MMF_DISABLE_THP 24 /* disable THP for all VMAs */ > -#define MMF_OOM_VICTIM 25 /* mm is the oom victim */ > #define MMF_OOM_REAP_QUEUED 26 /* mm was queued for oom_reaper */ > #define MMF_MULTIPROCESS 27 /* mm is shared between processes */ > /* > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 36355b162727..11291b99599f 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -732,10 +732,8 @@ static void mark_oom_victim(struct task_struct *tsk) > return; > > /* oom_mm is bound to the signal struct life time. */ > - if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) { > + if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) > mmgrab(tsk->signal->oom_mm); > - set_bit(MMF_OOM_VICTIM, &mm->flags); > - } > > /* > * Make sure that the task is woken up from uninterruptible sleep > Thank you for working on the new tests and cleanups. This series needs a cover-letter that explains why this series is needed that includes the information from this last patch. Please send v2 with a proper cover letter starting with why this series is necessary. If you did that, it would have reviewers job is lot easier. Also it appears you are combining new tests with cleanup patches. I think patches 2/3 and 3/3 can be a separate series and the new test can be a separate patch. thanks, -- Shuah
On Tue, May 10, 2022 at 8:51 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 5/9/22 9:00 PM, Suren Baghdasaryan wrote: > > With the last usage of MMF_OOM_VICTIM in exit_mmap gone, this flag is > > now unused and can be removed. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > include/linux/oom.h | 9 --------- > > include/linux/sched/coredump.h | 1 - > > mm/oom_kill.c | 4 +--- > > 3 files changed, 1 insertion(+), 13 deletions(-) > > > > diff --git a/include/linux/oom.h b/include/linux/oom.h > > index 6cdf0772dbae..25990e9d9e15 100644 > > --- a/include/linux/oom.h > > +++ b/include/linux/oom.h > > @@ -77,15 +77,6 @@ static inline bool tsk_is_oom_victim(struct task_struct * tsk) > > return tsk->signal->oom_mm; > > } > > > > -/* > > - * Use this helper if tsk->mm != mm and the victim mm needs a special > > - * handling. This is guaranteed to stay true after once set. > > - */ > > -static inline bool mm_is_oom_victim(struct mm_struct *mm) > > -{ > > - return test_bit(MMF_OOM_VICTIM, &mm->flags); > > -} > > - > > /* > > * Checks whether a page fault on the given mm is still reliable. > > * This is no longer true if the oom reaper started to reap the > > diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h > > index 4d9e3a656875..746f6cb07a20 100644 > > --- a/include/linux/sched/coredump.h > > +++ b/include/linux/sched/coredump.h > > @@ -70,7 +70,6 @@ static inline int get_dumpable(struct mm_struct *mm) > > #define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */ > > #define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */ > > #define MMF_DISABLE_THP 24 /* disable THP for all VMAs */ > > -#define MMF_OOM_VICTIM 25 /* mm is the oom victim */ > > #define MMF_OOM_REAP_QUEUED 26 /* mm was queued for oom_reaper */ > > #define MMF_MULTIPROCESS 27 /* mm is shared between processes */ > > /* > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > index 36355b162727..11291b99599f 100644 > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -732,10 +732,8 @@ static void mark_oom_victim(struct task_struct *tsk) > > return; > > > > /* oom_mm is bound to the signal struct life time. */ > > - if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) { > > + if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) > > mmgrab(tsk->signal->oom_mm); > > - set_bit(MMF_OOM_VICTIM, &mm->flags); > > - } > > > > /* > > * Make sure that the task is woken up from uninterruptible sleep > > > > Thank you for working on the new tests and cleanups. Thanks for the review! > > This series needs a cover-letter that explains why this series is needed > that includes the information from this last patch. > > Please send v2 with a proper cover letter starting with why this series > is necessary. If you did that, it would have reviewers job is lot easier. > > Also it appears you are combining new tests with cleanup patches. I think > patches 2/3 and 3/3 can be a separate series and the new test can be a > separate patch. I used the new selftest to test the patches but otherwise it's true, they are unrelated. I was debating whether to send them separately and with your blessing I'll split them up. Thanks, Suren. > > thanks, > -- Shuah
On Tue, May 10, 2022 at 6:08 AM Michal Hocko <mhocko@suse.com> wrote: > > On Mon 09-05-22 20:00:14, Suren Baghdasaryan wrote: > > With the last usage of MMF_OOM_VICTIM in exit_mmap gone, this flag is > > now unused and can be removed. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > LGTM > Acked-by: Michal Hocko <mhocko@suse.com> > > One question below > [...] > > diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h > > index 4d9e3a656875..746f6cb07a20 100644 > > --- a/include/linux/sched/coredump.h > > +++ b/include/linux/sched/coredump.h > > @@ -70,7 +70,6 @@ static inline int get_dumpable(struct mm_struct *mm) > > #define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */ > > #define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */ > > #define MMF_DISABLE_THP 24 /* disable THP for all VMAs */ > > -#define MMF_OOM_VICTIM 25 /* mm is the oom victim */ > > #define MMF_OOM_REAP_QUEUED 26 /* mm was queued for oom_reaper */ > > #define MMF_MULTIPROCESS 27 /* mm is shared between processes */ > > Have you consider renumbering the follow up flags so that we do not have > holes in there. Nothing really important but it can confuse somebody in > the future. Missed this note until now. I will renumber the constants to avoid confusion. Thanks, Suren. > > -- > Michal Hocko > SUSE Labs
diff --git a/include/linux/oom.h b/include/linux/oom.h index 6cdf0772dbae..25990e9d9e15 100644 --- a/include/linux/oom.h +++ b/include/linux/oom.h @@ -77,15 +77,6 @@ static inline bool tsk_is_oom_victim(struct task_struct * tsk) return tsk->signal->oom_mm; } -/* - * Use this helper if tsk->mm != mm and the victim mm needs a special - * handling. This is guaranteed to stay true after once set. - */ -static inline bool mm_is_oom_victim(struct mm_struct *mm) -{ - return test_bit(MMF_OOM_VICTIM, &mm->flags); -} - /* * Checks whether a page fault on the given mm is still reliable. * This is no longer true if the oom reaper started to reap the diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h index 4d9e3a656875..746f6cb07a20 100644 --- a/include/linux/sched/coredump.h +++ b/include/linux/sched/coredump.h @@ -70,7 +70,6 @@ static inline int get_dumpable(struct mm_struct *mm) #define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */ #define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */ #define MMF_DISABLE_THP 24 /* disable THP for all VMAs */ -#define MMF_OOM_VICTIM 25 /* mm is the oom victim */ #define MMF_OOM_REAP_QUEUED 26 /* mm was queued for oom_reaper */ #define MMF_MULTIPROCESS 27 /* mm is shared between processes */ /* diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 36355b162727..11291b99599f 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -732,10 +732,8 @@ static void mark_oom_victim(struct task_struct *tsk) return; /* oom_mm is bound to the signal struct life time. */ - if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) { + if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) mmgrab(tsk->signal->oom_mm); - set_bit(MMF_OOM_VICTIM, &mm->flags); - } /* * Make sure that the task is woken up from uninterruptible sleep
With the last usage of MMF_OOM_VICTIM in exit_mmap gone, this flag is now unused and can be removed. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- include/linux/oom.h | 9 --------- include/linux/sched/coredump.h | 1 - mm/oom_kill.c | 4 +--- 3 files changed, 1 insertion(+), 13 deletions(-)