Re: [PATCH 02/10] mm: convert core mm to mm_flags_*() accessors

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Sep 17, 2025 at 02:16:54AM +0200, Mateusz Guzik wrote:
> On Wed, Sep 17, 2025 at 1:57 AM Chris Mason <clm@meta.com> wrote:
> >
> > On Tue, 12 Aug 2025 16:44:11 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> >
> > > As part of the effort to move to mm->flags becoming a bitmap field, convert
> > > existing users to making use of the mm_flags_*() accessors which will, when
> > > the conversion is complete, be the only means of accessing mm_struct flags.
> > >
> > > This will result in the debug output being that of a bitmap output, which
> > > will result in a minor change here, but since this is for debug only, this
> > > should have no bearing.
> > >
> > > Otherwise, no functional changes intended.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> > [ ... ]
> >
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index 25923cfec9c6..17650f0b516e 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> >
> > [ ... ]
> >
> > > @@ -1251,7 +1251,7 @@ SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
> > >        * Check MMF_OOM_SKIP again under mmap_read_lock protection to ensure
> > >        * possible change in exit_mmap is seen
> > >        */
> > > -     if (!test_bit(MMF_OOM_SKIP, &mm->flags) && !__oom_reap_task_mm(mm))
> > > +     if (mm_flags_test(MMF_OOM_SKIP, mm) && !__oom_reap_task_mm(mm))
> > >               ret = -EAGAIN;
> > >       mmap_read_unlock(mm);
> > >
> >
> > Hi Lorzeno, I think we lost a ! here.
> >
> > claude found enough inverted logic in moved code that I did a new run with
> > a more explicit prompt for it, but this was the only new hit.
> >
>
> I presume conversion was done mostly manually?

Actually largely via sed/emacs find-replace. I'm not sure why this case
happened. But maybe it's one of the not 'largely' changes...

Human-in-the-middle is obviously subject to errors :)

>
> The way(tm) is to use coccinelle.
>
> I whipped out the following real quick and results look good:
>
> @@
> expression mm, bit;
> @@
>
> - test_bit(bit, &mm->flags)
> + mm_flags_test(bit, mm)

Thanks. Not sure it'd hit every case. But that's useful to know, could
presumably expand to hit others.

I will be changing VMA flags when my review load finally allows me to so knowing
this is useful...

Cheers, Lorenzo

>
> $ spatch --sp-file mmbit.cocci mm/oom_kill.c
> [snip]
> @@ -892,7 +892,7 @@ static bool task_will_free_mem(struct ta
>          * This task has already been drained by the oom reaper so there are
>          * only small chances it will free some more
>          */
> -       if (test_bit(MMF_OOM_SKIP, &mm->flags))
> +       if (mm_flags_test(MMF_OOM_SKIP, mm))
>                 return false;
>
>         if (atomic_read(&mm->mm_users) <= 1)
> @@ -1235,7 +1235,7 @@ SYSCALL_DEFINE2(process_mrelease, int, p
>                 reap = true;
>         else {
>                 /* Error only if the work has not been done already */
> -               if (!test_bit(MMF_OOM_SKIP, &mm->flags))
> +               if (!mm_flags_test(MMF_OOM_SKIP, mm))
>                         ret = -EINVAL;
>         }
>         task_unlock(p);
> @@ -1251,7 +1251,7 @@ SYSCALL_DEFINE2(process_mrelease, int, p
>          * Check MMF_OOM_SKIP again under mmap_read_lock protection to ensure
>          * possible change in exit_mmap is seen
>          */
> -       if (!test_bit(MMF_OOM_SKIP, &mm->flags) && !__oom_reap_task_mm(mm))
> +       if (!mm_flags_test(MMF_OOM_SKIP, mm) && !__oom_reap_task_mm(mm))
>                 ret = -EAGAIN;
>         mmap_read_unlock(mm);