linux-kernel - Re: [External] Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACSyD1OcKg7nc54OXe1F+4oPort7jtG5wu1J75z7_CZNTkrRDw@mail.gmail.com>
Date: Thu, 19 Jun 2025 12:36:44 +0800
From: Zhongkun He <hezhongkun.hzk@...edance.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: Andrew Morton <akpm@...ux-foundation.org>, mhocko@...e.com, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

On Thu, Jun 19, 2025 at 12:17 PM Kent Overstreet
<kent.overstreet@...ux.dev> wrote:
>
> On Thu, Jun 19, 2025 at 11:17:58AM +0800, Zhongkun He wrote:
> > On Thu, Jun 19, 2025 at 8:07 AM Kent Overstreet
> > <kent.overstreet@...ux.dev> wrote:
> > >
> > > On Wed, Jun 18, 2025 at 04:43:34PM -0700, Andrew Morton wrote:
> > > > On Wed, 18 Jun 2025 15:03:28 +0800 Zhongkun He <hezhongkun.hzk@...edance.com> wrote:
> > > >
> > > > > The variable name oldflags can indeed be misleading, because
> > > > > it does not store the complete original value of flags.
> > > > > Instead, it records which flags from the given set are not
> > > > > currently set. So rename it.
> > > > >
> > > >
> > > > Your email client is mangling the patches in strange ways.  Please send
> > > > yourself a patch, figure out why it didn't apply?
> > > >
> > > > > --- a/include/linux/sched/mm.h
> > > > > +++ b/include/linux/sched/mm.h
> > > > > @@ -322,21 +322,21 @@ static inline void might_alloc(gfp_t gfp_mask)
> > > > >  }
> > > > >
> > > > >  /**
> > > > > - * memalloc_flags_save - Add a PF_* flag to current->flags, save old value
> > > > > + * memalloc_flags_save - Add a PF_* flag to current->flags, return saved flags mask
> > > > >   *
> > > > >   * This allows PF_* flags to be conveniently added, irrespective of current
> > > > >   * value, and then the old version restored with memalloc_flags_restore().
> > > > >   */
> > > > > -static inline unsigned memalloc_flags_save(unsigned flags)
> > > > > +static inline unsigned int memalloc_flags_save(unsigned int flags_mask)
> > > > >  {
> > > > > -   unsigned oldflags = ~current->flags & flags;
> > > > > -   current->flags |= flags;
> > > > > -   return oldflags;
> > > > > +   unsigned int saved_flags_mask = ~current->flags & flags_mask;
> > > > > +
> > > > > +   current->flags |= flags_mask;
> > > > > +   return saved_flags_mask;
> > > > >  }
> > > > >
> > > > > -static inline void memalloc_flags_restore(unsigned flags)
> > > > > +static inline void memalloc_flags_restore(unsigned int flags_mask)
> > > > >  {
> > > > > -   current->flags &= ~flags;
> > > > > +   current->flags &= ~flags_mask;
> > > > >  }
> > > >
> > > > I guess so.  Maybe.  A bit.  Kent, what do you think?
> > >
> > > Eesh, seems like pointless verbosity to me. Maybe don't change it if it
> > > doesn't need to be changed?
> >
> > Hi Kent, thanks for your feedback.
> > How about this version,  only change the 'old' to 'saved'.
> > The function does not return the old current->flags value. Instead,
> > it returns the subset of flags that were not previously set in current->flags,
> > so they can later be cleared by memalloc_flags_restore(). The name savedflags
> > makes this behavior clearer and avoids confusion.
>
> Why change it at all? The returned flags parameter is opaque state that
> should only be used by memalloc_flags_restore(), it's not something the
> caller should be looking at.
>

Hi Kent

You're right that the returned flags are opaque and only meant for
memalloc_flags_restore(),
not for inspection by the caller.

My motivation for renaming was that the term oldflags might suggest it
contains the original
current->flags value, which could be misleading when reading the code.
Since it actually
represents a subset of flags to be cleared later, savedflags seems clearer.

That said, if the current naming is considered fine given its opaque
nature, I'm happy to
drop the rename.

Thanks,
Zhongkun.

> >
> >  /**
> > - * memalloc_flags_save - Add a PF_* flag to current->flags, save old value
> > + * memalloc_flags_save - Add a PF_* flag to current->flags, return saved flags
> >   *
> >   * This allows PF_* flags to be conveniently added, irrespective of current
> >   * value, and then the old version restored with memalloc_flags_restore().
> >   */
> >  static inline unsigned memalloc_flags_save(unsigned flags)
> >  {
> > -       unsigned oldflags = ~current->flags & flags;
> > +       unsigned savedflags = ~current->flags & flags;
> >         current->flags |= flags;
> > -       return oldflags;
> > +       return savedflags;
> >  }
> >
> > Thanks,
> > Zhongkun