| Message ID | 20250212025843.80283-3-liuye@kylinos.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Optimize folio_order. | expand |
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@kylinos.cn> > --- > 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.
On 2/12/2025 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@kylinos.cn> > --- > 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) This folio order calculation is only valid for !large folios. When it's a single page (not a large folio), the memory is interpreted as struct page. struct folio { ... union { struct { unsigned long _flags_1; unsigned long _head_1; /* public: */ atomic_t _large_mapcount; atomic_t _entire_mapcount; atomic_t _nr_pages_mapped; atomic_t _pincount; #ifdef CONFIG_64BIT unsigned int _folio_nr_pages; #endif /* private: the union with struct page is transitional */ }; struct page __page_1; }; ... } I feel this to be risky, considering someone may directly use FOLIO_ORDER() macro without folio_test_large() check. Correct macro should look like: #define FOLIO_ORDER(folio) (folio_test_large(folio) ? ((folio)->_flags_1 & 0xff) : 0) Thanks, Shivank > + > /* > * 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 > } >
在 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@kylinos.cn> >> --- >> 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. Thanks， Liu Ye
在 2025/2/12 13:40, Shivank Garg 写道: > On 2/12/2025 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@kylinos.cn> >> --- >> 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) > > This folio order calculation is only valid for !large folios. > When it's a single page (not a large folio), the memory is interpreted as struct page. > > struct folio { > ... > union { > struct { > unsigned long _flags_1; > unsigned long _head_1; > /* public: */ > atomic_t _large_mapcount; > atomic_t _entire_mapcount; > atomic_t _nr_pages_mapped; > atomic_t _pincount; > #ifdef CONFIG_64BIT > unsigned int _folio_nr_pages; > #endif > /* private: the union with struct page is transitional */ > }; > struct page __page_1; > }; > ... > } > > I feel this to be risky, considering someone may directly use FOLIO_ORDER() macro > without folio_test_large() check. > > Correct macro should look like: > > #define FOLIO_ORDER(folio) (folio_test_large(folio) ? ((folio)->_flags_1 & 0xff) : 0) > Yes, this is safer. At present, the positions using FOLIO-ORDER have been checked using folio_test_1arge or test-bit (PG_cead,&folio ->flags), and these positions may need to be simplified. > > Thanks, > Shivank >> + >> /* >> * 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 >> } >> >
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@kylinos.cn> >>> --- >>> 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 > > >
On Wed, Feb 12, 2025 at 10:58:43AM +0800, 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. No.
On 12.02.25 03:58, 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@kylinos.cn> I have something different (better) in the works: https://lore.kernel.org/linux-mm/20240829165627.2256514-3-david@redhat.com/
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 }
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@kylinos.cn> --- include/linux/mm.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)