On Mon, Mar 21, 2022 at 10:21:24AM -0400, Zi Yan wrote:
> From: Zi Yan <[email protected]>
>
> It sets memcg information for the pages after the split. A new parameter
> new_order is added to tell the new page order, always 0 for now. It
> prepares for upcoming changes to support split huge page to any lower
> order.
>
> Signed-off-by: Zi Yan <[email protected]>
> ---
> include/linux/memcontrol.h | 2 +-
> mm/huge_memory.c | 2 +-
> mm/memcontrol.c | 10 +++++-----
> mm/page_alloc.c | 2 +-
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 89b14729d59f..e71189454bf0 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1116,7 +1116,7 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
> rcu_read_unlock();
> }
>
> -void split_page_memcg(struct page *head, unsigned int nr);
> +void split_page_memcg(struct page *head, unsigned int nr, unsigned int new_order);
It looks a bit inconsistent, can't we switch to use either nr or order for both
arguments? The latter is preferable.
Other than that, the patch looks good to me.
Thanks!
On 21 Mar 2022, at 14:57, Roman Gushchin wrote:
> On Mon, Mar 21, 2022 at 10:21:24AM -0400, Zi Yan wrote:
>> From: Zi Yan <[email protected]>
>>
>> It sets memcg information for the pages after the split. A new parameter
>> new_order is added to tell the new page order, always 0 for now. It
>> prepares for upcoming changes to support split huge page to any lower
>> order.
>>
>> Signed-off-by: Zi Yan <[email protected]>
>> ---
>> include/linux/memcontrol.h | 2 +-
>> mm/huge_memory.c | 2 +-
>> mm/memcontrol.c | 10 +++++-----
>> mm/page_alloc.c | 2 +-
>> 4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 89b14729d59f..e71189454bf0 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -1116,7 +1116,7 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
>> rcu_read_unlock();
>> }
>>
>> -void split_page_memcg(struct page *head, unsigned int nr);
>> +void split_page_memcg(struct page *head, unsigned int nr, unsigned int new_order);
>
> It looks a bit inconsistent, can't we switch to use either nr or order for both
> arguments? The latter is preferable.
Yes. Will change it to new_nr to be consistent.
>
> Other than that, the patch looks good to me.
Thank you for the review.
--
Best Regards,
Yan, Zi
On Mon, Mar 21, 2022 at 03:07:46PM -0400, Zi Yan wrote:
> Yes. Will change it to new_nr to be consistent.
uh, you're going to call ilog2?
I think this would look less inconsistent if 'nr' were an unsigned long
(how long until we need 16GB pages? Think PPC already supports those)
On 21 Mar 2022, at 15:54, Matthew Wilcox wrote:
> On Mon, Mar 21, 2022 at 03:07:46PM -0400, Zi Yan wrote:
>> Yes. Will change it to new_nr to be consistent.
>
> uh, you're going to call ilog2?
fortunately, no. Inside split_page_memcg(), I probably
need to add VM_BUG_ON(nr % new_nr != 0) to make sure
new_nr is a divisor of nr, since there are a couple
of nr / new_nr operations. Otherwise, new_nr works.
>
> I think this would look less inconsistent if 'nr' were an unsigned long
> (how long until we need 16GB pages? Think PPC already supports those)
--
Best Regards,
Yan, Zi