Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition

[Dev Jain]

On 12/02/25 12:37 pm, liuye wrote:
> 在 2025/2/12 13:12, Dev Jain 写道:
> > On 12/02/25 8:28 am, Liu Ye wrote:
> > > There are multiple locations in mm.h where (folio->_flags_1 & 0xff) is
> > > used. Write it as a macro definition to improve the readability and
> > > maintainability of the code.
> > >
> > > Signed-off-by: Liu Ye <liuye@xxxxxxxxxx>
> > > ---
> > >    include/linux/mm.h | 10 ++++++----
> > >    1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 7b1068ddcbb7..750e75f45557 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -1098,6 +1098,8 @@ int vma_is_stack_for_current(struct vm_area_struct *vma);
> > >    struct mmu_gather;
> > >    struct inode;
> > >    +#define FOLIO_ORDER(folio) ((folio)->_flags_1 & 0xff)
> > > +
> > >    /*
> > >     * compound_order() can be called without holding a reference, which means
> > >     * that niceties like page_folio() don't work.  These callers should be
> > > @@ -1111,7 +1113,7 @@ static inline unsigned int compound_order(struct page *page)
> > >          if (!test_bit(PG_head, &folio->flags))
> > >            return 0;
> > > -    return folio->_flags_1 & 0xff;
> > > +    return FOLIO_ORDER(folio);
> > >    }
> > >      /**
> > > @@ -1127,7 +1129,7 @@ static inline unsigned int folio_order(const struct folio *folio)
> > >    {
> > >        if (!folio_test_large(folio))
> > >            return 0;
> > > -    return folio->_flags_1 & 0xff;
> > > +    return FOLIO_ORDER(folio);
> > >    }
> > >      #include <linux/huge_mm.h>
> > > @@ -2061,7 +2063,7 @@ static inline long folio_nr_pages(const struct folio *folio)
> > >    #ifdef CONFIG_64BIT
> > >        return folio->_folio_nr_pages;
> > >    #else
> > > -    return 1L << (folio->_flags_1 & 0xff);
> > > +    return 1L << FOLIO_ORDER(folio);
> > >    #endif
> > >    }
> > >    @@ -2086,7 +2088,7 @@ static inline unsigned long compound_nr(struct page *page)
> > >    #ifdef CONFIG_64BIT
> > >        return folio->_folio_nr_pages;
> > >    #else
> > > -    return 1L << (folio->_flags_1 & 0xff);
> > > +    return 1L << FOLIO_ORDER(folio);
> > >    #endif
> > >    }
> > >    
> >
> > Personally I do not think this is improving readability. You are introducing one more macro for people to decipher instead of directly seeing folio->_flags_1 & 0xff. This is similar to whether to write
> > if (x) => do_stuff(), or if (x != 0) => do_stuff(). The former is more "readable" by convention but the latter makes it easier and obvious to understand.
> Or simply for maintenance purposes, if the meaning of a bit changes, only the macro definition needs to be modified.

Well, then let us wait for that time to come :) Personally I am not a 
fan of over-abstracting, especially when it is just a single line; one 
benefit I have seen of writing the way it is written right now, is that 
I actually get reminded where the folio order is actually stored. I have 
no objection on getting this patch applied, if someone else thinks this 
is a fruitful abstraction. In any case, you do need to come up with a 
better name than FOLIO_ORDER, as it is confusing.

> Thanks，
> Liu Ye

>