* Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition
2025-02-12 2:58 ` [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition Liu Ye
@ 2025-02-12 5:12 ` Dev Jain
2025-02-12 5:40 ` Shivank Garg
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Dev Jain @ 2025-02-12 5:12 UTC (permalink / raw)
To: Liu Ye, brauner, dhowells, akpm; +Cc: linux-kernel, linux-mm
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition
2025-02-12 2:58 ` [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition Liu Ye
2025-02-12 5:12 ` Dev Jain
@ 2025-02-12 5:40 ` Shivank Garg
2025-02-12 7:11 ` liuye
[not found] ` <1739340112672653.3.seg@mailgw.kylinos.cn>
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Shivank Garg @ 2025-02-12 5:40 UTC (permalink / raw)
To: Liu Ye, brauner, dhowells, akpm; +Cc: linux-kernel, linux-mm
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
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition
2025-02-12 5:40 ` Shivank Garg
@ 2025-02-12 7:11 ` liuye
0 siblings, 0 replies; 13+ messages in thread
From: liuye @ 2025-02-12 7:11 UTC (permalink / raw)
To: Shivank Garg, brauner, dhowells, akpm; +Cc: linux-kernel, linux-mm
在 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
>> }
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <1739340112672653.3.seg@mailgw.kylinos.cn>]
* Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition
[not found] ` <1739340112672653.3.seg@mailgw.kylinos.cn>
@ 2025-02-12 7:07 ` liuye
2025-02-12 9:06 ` Dev Jain
0 siblings, 1 reply; 13+ messages in thread
From: liuye @ 2025-02-12 7:07 UTC (permalink / raw)
To: Dev Jain, brauner, dhowells, akpm; +Cc: linux-kernel, linux-mm
在 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition
2025-02-12 7:07 ` liuye
@ 2025-02-12 9:06 ` Dev Jain
0 siblings, 0 replies; 13+ messages in thread
From: Dev Jain @ 2025-02-12 9:06 UTC (permalink / raw)
To: liuye, brauner, dhowells, akpm; +Cc: linux-kernel, linux-mm
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
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition
2025-02-12 2:58 ` [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition Liu Ye
` (2 preceding siblings ...)
[not found] ` <1739340112672653.3.seg@mailgw.kylinos.cn>
@ 2025-02-12 12:36 ` Matthew Wilcox
2025-02-12 16:22 ` David Hildenbrand
4 siblings, 0 replies; 13+ messages in thread
From: Matthew Wilcox @ 2025-02-12 12:36 UTC (permalink / raw)
To: Liu Ye; +Cc: brauner, dhowells, akpm, linux-kernel, linux-mm
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition
2025-02-12 2:58 ` [PATCH 2/2] mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition Liu Ye
` (3 preceding siblings ...)
2025-02-12 12:36 ` Matthew Wilcox
@ 2025-02-12 16:22 ` David Hildenbrand
4 siblings, 0 replies; 13+ messages in thread
From: David Hildenbrand @ 2025-02-12 16:22 UTC (permalink / raw)
To: Liu Ye, brauner, dhowells, akpm; +Cc: linux-kernel, linux-mm
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/
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread