[PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[Zhongkun He]

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.

Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com>
---
 include/linux/sched/mm.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index b13474825130..6b4b54e8db92 100644
--- 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;
 }
 
 /**
-- 
2.39.5

Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[Andrew Morton]

On Wed, 18 Jun 2025 15:03:28 +0800 Zhongkun He <hezhongkun.hzk@bytedance.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?

Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[Kent Overstreet]

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@bytedance.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?

Re: [External] Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[Zhongkun He]

On Thu, Jun 19, 2025 at 8:07 AM Kent Overstreet
<kent.overstreet@linux.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@bytedance.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.

 /**
- * 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

Re: [External] Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[Kent Overstreet]

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@linux.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@bytedance.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.

> 
>  /**
> - * 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

Re: [External] Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[Zhongkun He]

On Thu, Jun 19, 2025 at 12:17 PM Kent Overstreet
<kent.overstreet@linux.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@linux.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@bytedance.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

Re: [External] Re: [PATCH] mm: rename the oldflags and parameter in memalloc_flags_*()

[Zhongkun He]

On Thu, Jun 19, 2025 at 7:43 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 18 Jun 2025 15:03:28 +0800 Zhongkun He <hezhongkun.hzk@bytedance.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?

Thank you for pointing that out. I’ll check the email formatting on my side.
Sorry for the trouble caused.

>
> > --- 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?