[PATCH 0/2] mm: Optimize folio_order.

[PATCH 0/2] mm: Optimize folio_order.

[Liu Ye]

Patches about folio_order.
1. Delete __folio_order and use folio_order directly.
2. Write folio->_flags_1 & 0xff as a macro definition.

Liu Ye (2):
  mm/folio_queue: Delete __folio_order and use folio_order directly
  mm/mm.h: Write folio->_flags_1 & 0xff as a macro definition

 include/linux/folio_queue.h | 12 +++---------
 include/linux/mm.h          | 10 ++++++----
 2 files changed, 9 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH 1/2] mm/folio_queue: Delete __folio_order and use folio_order directly

[Liu Ye]

__folio_order is the same as folio_order, remove __folio_order and then
just include mm.h and use folio_order directly.

Signed-off-by: Liu Ye <liuye@kylinos.cn>
---
 include/linux/folio_queue.h | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/include/linux/folio_queue.h b/include/linux/folio_queue.h
index 4d3f8074c137..45ad2408a80c 100644
--- a/include/linux/folio_queue.h
+++ b/include/linux/folio_queue.h
@@ -15,6 +15,7 @@
 #define _LINUX_FOLIO_QUEUE_H
 
 #include <linux/pagevec.h>
+#include <linux/mm.h>
 
 /*
  * Segment in a queue of running buffers.  Each segment can hold a number of
@@ -216,13 +217,6 @@ static inline void folioq_unmark3(struct folio_queue *folioq, unsigned int slot)
 	clear_bit(slot, &folioq->marks3);
 }
 
-static inline unsigned int __folio_order(struct folio *folio)
-{
-	if (!folio_test_large(folio))
-		return 0;
-	return folio->_flags_1 & 0xff;
-}
-
 /**
  * folioq_append: Add a folio to a folio queue segment
  * @folioq: The segment to add to
@@ -241,7 +235,7 @@ static inline unsigned int folioq_append(struct folio_queue *folioq, struct foli
 	unsigned int slot = folioq->vec.nr++;
 
 	folioq->vec.folios[slot] = folio;
-	folioq->orders[slot] = __folio_order(folio);
+	folioq->orders[slot] = folio_order(folio);
 	return slot;
 }
 
@@ -263,7 +257,7 @@ static inline unsigned int folioq_append_mark(struct folio_queue *folioq, struct
 	unsigned int slot = folioq->vec.nr++;
 
 	folioq->vec.folios[slot] = folio;
-	folioq->orders[slot] = __folio_order(folio);
+	folioq->orders[slot] = folio_order(folio);
 	folioq_mark(folioq, slot);
 	return slot;
 }
-- 
2.25.1

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

[Liu Ye]

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
 }
 
-- 
2.25.1

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

[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.

Re: [PATCH 1/2] mm/folio_queue: Delete __folio_order and use folio_order directly

[Shivank Garg]

On 2/12/2025 8:28 AM, Liu Ye wrote:
> __folio_order is the same as folio_order, remove __folio_order and then
> just include mm.h and use folio_order directly.
> 
> Signed-off-by: Liu Ye <liuye@kylinos.cn>
> ---
>  include/linux/folio_queue.h | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/folio_queue.h b/include/linux/folio_queue.h
> index 4d3f8074c137..45ad2408a80c 100644
> --- a/include/linux/folio_queue.h
> +++ b/include/linux/folio_queue.h
> @@ -15,6 +15,7 @@
>  #define _LINUX_FOLIO_QUEUE_H
>  
>  #include <linux/pagevec.h>
> +#include <linux/mm.h>
>  
>  /*
>   * Segment in a queue of running buffers.  Each segment can hold a number of
> @@ -216,13 +217,6 @@ static inline void folioq_unmark3(struct folio_queue *folioq, unsigned int slot)
>  	clear_bit(slot, &folioq->marks3);
>  }
>  
> -static inline unsigned int __folio_order(struct folio *folio)
> -{
> -	if (!folio_test_large(folio))
> -		return 0;
> -	return folio->_flags_1 & 0xff;
> -}
> -
>  /**
>   * folioq_append: Add a folio to a folio queue segment
>   * @folioq: The segment to add to
> @@ -241,7 +235,7 @@ static inline unsigned int folioq_append(struct folio_queue *folioq, struct foli
>  	unsigned int slot = folioq->vec.nr++;
>  
>  	folioq->vec.folios[slot] = folio;
> -	folioq->orders[slot] = __folio_order(folio);
> +	folioq->orders[slot] = folio_order(folio);
>  	return slot;
>  }
>  
> @@ -263,7 +257,7 @@ static inline unsigned int folioq_append_mark(struct folio_queue *folioq, struct
>  	unsigned int slot = folioq->vec.nr++;
>  
>  	folioq->vec.folios[slot] = folio;
> -	folioq->orders[slot] = __folio_order(folio);
> +	folioq->orders[slot] = folio_order(folio);
>  	folioq_mark(folioq, slot);
>  	return slot;
>  }


Reviewed-by: Shivank Garg <shivankg@amd.com>

Thanks,
Shivank

Re: [PATCH 1/2] mm/folio_queue: Delete __folio_order and use folio_order directly

[Dev Jain]

On 12/02/25 8:28 am, Liu Ye wrote:
> __folio_order is the same as folio_order, remove __folio_order and then
> just include mm.h and use folio_order directly.
> 
> Signed-off-by: Liu Ye <liuye@kylinos.cn>
> ---
>   include/linux/folio_queue.h | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/folio_queue.h b/include/linux/folio_queue.h
> index 4d3f8074c137..45ad2408a80c 100644
> --- a/include/linux/folio_queue.h
> +++ b/include/linux/folio_queue.h
> @@ -15,6 +15,7 @@
>   #define _LINUX_FOLIO_QUEUE_H
>   
>   #include <linux/pagevec.h>
> +#include <linux/mm.h>
>   
>   /*
>    * Segment in a queue of running buffers.  Each segment can hold a number of
> @@ -216,13 +217,6 @@ static inline void folioq_unmark3(struct folio_queue *folioq, unsigned int slot)
>   	clear_bit(slot, &folioq->marks3);
>   }
>   
> -static inline unsigned int __folio_order(struct folio *folio)
> -{
> -	if (!folio_test_large(folio))
> -		return 0;
> -	return folio->_flags_1 & 0xff;
> -}
> -
>   /**
>    * folioq_append: Add a folio to a folio queue segment
>    * @folioq: The segment to add to
> @@ -241,7 +235,7 @@ static inline unsigned int folioq_append(struct folio_queue *folioq, struct foli
>   	unsigned int slot = folioq->vec.nr++;
>   
>   	folioq->vec.folios[slot] = folio;
> -	folioq->orders[slot] = __folio_order(folio);
> +	folioq->orders[slot] = folio_order(folio);
>   	return slot;
>   }
>   
> @@ -263,7 +257,7 @@ static inline unsigned int folioq_append_mark(struct folio_queue *folioq, struct
>   	unsigned int slot = folioq->vec.nr++;
>   
>   	folioq->vec.folios[slot] = folio;
> -	folioq->orders[slot] = __folio_order(folio);
> +	folioq->orders[slot] = folio_order(folio);
>   	folioq_mark(folioq, slot);
>   	return slot;
>   }

This looks like a reasonable change to make, since it avoids code 
duplication. Please consider:

Reviewed-by: Dev Jain <dev.jain@arm.com>

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

[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)


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

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

[liuye]

在 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

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

[liuye]

在 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
>>  }
>>  
> 

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

> 
> 

Re: [PATCH 1/2] mm/folio_queue: Delete __folio_order and use folio_order directly

[David Howells]

Liu Ye <liuye@kylinos.cn> wrote:

> __folio_order is the same as folio_order, remove __folio_order and then
> just include mm.h and use folio_order directly.
> 
> Signed-off-by: Liu Ye <liuye@kylinos.cn>

Acked-by: David Howells <dhowells@redhat.com>

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

[Matthew Wilcox]

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.

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

[David Hildenbrand]

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