[PATCH 0/3] add page_ext_data to get client data in page_ext

[PATCH 0/3] add page_ext_data to get client data in page_ext

[Kemeng Shi]

Current client get data from page_ext by adding offset which is auto
generated in page_ext core and expose the data layout design insdie
page_ext core. This series adds a page_ext_data to hide offset from
client. Thanks!

Kemeng Shi (3):
  mm/page_ext: add common function to get client data from page_ext
  mm/page_ext: use page_ext_data helper in page_table_check
  mm/page_ext: use page_ext_data helper in page_owner

 include/linux/page_ext.h | 6 ++++++
 mm/page_owner.c          | 2 +-
 mm/page_table_check.c    | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.30.0

[PATCH 1/3] mm/page_ext: add common function to get client data from page_ext

[Kemeng Shi]

Add common page_ext_data function to get client data. This could hide
offset which is auto generated in page_ext core and expose the desgin
of page_ext data layout.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 include/linux/page_ext.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h
index 8dd21d87e0da..be98564191e6 100644
--- a/include/linux/page_ext.h
+++ b/include/linux/page_ext.h
@@ -81,6 +81,12 @@ static inline void page_ext_init(void)
 extern struct page_ext *page_ext_get(struct page *page);
 extern void page_ext_put(struct page_ext *page_ext);
 
+static inline void *page_ext_data(struct page_ext *page_ext,
+				  struct page_ext_operations *ops)
+{
+	return (void *)(page_ext) + ops->offset;
+}
+
 static inline struct page_ext *page_ext_next(struct page_ext *curr)
 {
 	void *next = curr;
-- 
2.30.0

[PATCH 2/3] mm/page_ext: use page_ext_data helper in page_table_check

[Kemeng Shi]

use page_ext_data helper in page_table_check to avoid access offset
directly.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/page_table_check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_table_check.c b/mm/page_table_check.c
index 84c8163984e5..46e77c12c81e 100644
--- a/mm/page_table_check.c
+++ b/mm/page_table_check.c
@@ -51,7 +51,7 @@ struct page_ext_operations page_table_check_ops = {
 static struct page_table_check *get_page_table_check(struct page_ext *page_ext)
 {
 	BUG_ON(!page_ext);
-	return (void *)(page_ext) + page_table_check_ops.offset;
+	return page_ext_data(page_ext, &page_table_check_ops);
 }
 
 /*
-- 
2.30.0

[PATCH 3/3] mm/page_ext: use page_ext_data helper in page_owner

[Kemeng Shi]

Use page_ext_data helper in page_owner to avoid access offset directly.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/page_owner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index c93baef0148f..4e2723e1b300 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -104,7 +104,7 @@ struct page_ext_operations page_owner_ops = {
 
 static inline struct page_owner *get_page_owner(struct page_ext *page_ext)
 {
-	return (void *)page_ext + page_owner_ops.offset;
+	return page_ext_data(page_ext, &page_owner_ops);
 }
 
 static noinline depot_stack_handle_t save_stack(gfp_t flags)
-- 
2.30.0

Re: [PATCH 0/3] add page_ext_data to get client data in page_ext

[Mike Rapoport]

On Tue, Jul 18, 2023 at 10:58:09PM +0800, Kemeng Shi wrote:
> Current client get data from page_ext by adding offset which is auto
> generated in page_ext core and expose the data layout design insdie
> page_ext core. This series adds a page_ext_data to hide offset from
> client. Thanks!

Implementers of page_ext_operations are anyway intimately related to
page_ext, so I'm not convinced this has any value.
 
> Kemeng Shi (3):
>   mm/page_ext: add common function to get client data from page_ext
>   mm/page_ext: use page_ext_data helper in page_table_check
>   mm/page_ext: use page_ext_data helper in page_owner
> 
>  include/linux/page_ext.h | 6 ++++++
>  mm/page_owner.c          | 2 +-
>  mm/page_table_check.c    | 2 +-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> -- 
> 2.30.0
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 0/3] add page_ext_data to get client data in page_ext

[Kemeng Shi]

on 7/19/2023 5:44 PM, Mike Rapoport wrote:
> On Tue, Jul 18, 2023 at 10:58:09PM +0800, Kemeng Shi wrote:
>> Current client get data from page_ext by adding offset which is auto
>> generated in page_ext core and expose the data layout design insdie
>> page_ext core. This series adds a page_ext_data to hide offset from
>> client. Thanks!
> 
> Implementers of page_ext_operations are anyway intimately related to
> page_ext, so I'm not convinced this has any value.
>  
Hi Mike, thanks for reply. I thinks page_ext_operations can be futher splited
into public part which used by client to simply register and private part which
only page_ext core cares and should not be accessed by client directly
to reduce decoupling. This series makes offset to be private which client
doesn't really care to hide data layout inside page_ext core from client.
There are some concrete gains I can list for now:
1. Future client cound call page_ext_data directly instead of define a
new function like get_page_owner to get it's data.
2. No change to client if layout of page_ext data change.
I hope this could be more convincing to you now.
Thanks!

-- 
Best wishes
Kemeng Shi

Re: [PATCH 0/3] add page_ext_data to get client data in page_ext

[Mike Rapoport]

Hi,

On Thu, Jul 20, 2023 at 10:38:39AM +0800, Kemeng Shi wrote:
> 
> on 7/19/2023 5:44 PM, Mike Rapoport wrote:
> > On Tue, Jul 18, 2023 at 10:58:09PM +0800, Kemeng Shi wrote:
> >> Current client get data from page_ext by adding offset which is auto
> >> generated in page_ext core and expose the data layout design insdie
> >> page_ext core. This series adds a page_ext_data to hide offset from
> >> client. Thanks!
> > 
> > Implementers of page_ext_operations are anyway intimately related to
> > page_ext, so I'm not convinced this has any value.
> >  
> Hi Mike, thanks for reply. I thinks page_ext_operations can be futher splited
> into public part which used by client to simply register and private part which
> only page_ext core cares and should not be accessed by client directly
> to reduce decoupling.

It would be easier to justify changes in this series if they were a part of
the refactoring you describe here.

> This series makes offset to be private which client
> doesn't really care to hide data layout inside page_ext core from client.
> There are some concrete gains I can list for now:
> 1. Future client cound call page_ext_data directly instead of define a
> new function like get_page_owner to get it's data.
> 2. No change to client if layout of page_ext data change.

These should be a part of the changelog.

> I hope this could be more convincing to you now.
> Thanks!
> 
> -- 
> Best wishes
> Kemeng Shi
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 0/3] add page_ext_data to get client data in page_ext

[Kemeng Shi]

on 7/20/2023 1:37 PM, Mike Rapoport wrote:
> Hi,
> 
> On Thu, Jul 20, 2023 at 10:38:39AM +0800, Kemeng Shi wrote:
>>
>> on 7/19/2023 5:44 PM, Mike Rapoport wrote:
>>> On Tue, Jul 18, 2023 at 10:58:09PM +0800, Kemeng Shi wrote:
>>>> Current client get data from page_ext by adding offset which is auto
>>>> generated in page_ext core and expose the data layout design insdie
>>>> page_ext core. This series adds a page_ext_data to hide offset from
>>>> client. Thanks!
>>>
>>> Implementers of page_ext_operations are anyway intimately related to
>>> page_ext, so I'm not convinced this has any value.
>>>  
>> Hi Mike, thanks for reply. I thinks page_ext_operations can be futher splited
>> into public part which used by client to simply register and private part which
>> only page_ext core cares and should not be accessed by client directly
>> to reduce decoupling.
> 
> It would be easier to justify changes in this series if they were a part of
> the refactoring you describe here.
Actually, it's not the refactoring trigger this. I found offset used
in client code while I could not find intialization of offset in client.
After some search, I found how offset is generated in page_ext core and
it's more like a page_ext internal behavior. Feel it's better to reduce
dependency from upper level client to low level internal implementation.

>> This series makes offset to be private which client
>> doesn't really care to hide data layout inside page_ext core from client.
>> There are some concrete gains I can list for now:
>> 1. Future client cound call page_ext_data directly instead of define a
>> new function like get_page_owner to get it's data.
>> 2. No change to client if layout of page_ext data change.
> 
> These should be a part of the changelog.
Yes, it's better to highlight the gains. This series was taken into the tree.
I'm not sure if I need send a v2 to include this or Andrew could add this
when code merged to more stable tree.
>> I hope this could be more convincing to you now.
>> Thanks!
>>
>> -- 
>> Best wishes
>> Kemeng Shi
>>
> 

-- 
Best wishes
Kemeng Shi

Re: [PATCH 0/3] add page_ext_data to get client data in page_ext

[Andrew Morton]

On Thu, 20 Jul 2023 08:37:36 +0300 Mike Rapoport <rppt@kernel.org> wrote:

> Hi,
> 
> On Thu, Jul 20, 2023 at 10:38:39AM +0800, Kemeng Shi wrote:
> > 
> > on 7/19/2023 5:44 PM, Mike Rapoport wrote:
> > > On Tue, Jul 18, 2023 at 10:58:09PM +0800, Kemeng Shi wrote:
> > >> Current client get data from page_ext by adding offset which is auto
> > >> generated in page_ext core and expose the data layout design insdie
> > >> page_ext core. This series adds a page_ext_data to hide offset from
> > >> client. Thanks!
> > > 
> > > Implementers of page_ext_operations are anyway intimately related to
> > > page_ext, so I'm not convinced this has any value.
> > >  
> > Hi Mike, thanks for reply. I thinks page_ext_operations can be futher splited
> > into public part which used by client to simply register and private part which
> > only page_ext core cares and should not be accessed by client directly
> > to reduce decoupling.
> 
> It would be easier to justify changes in this series if they were a part of
> the refactoring you describe here.
> 
> > This series makes offset to be private which client
> > doesn't really care to hide data layout inside page_ext core from client.
> > There are some concrete gains I can list for now:
> > 1. Future client cound call page_ext_data directly instead of define a
> > new function like get_page_owner to get it's data.
> > 2. No change to client if layout of page_ext data change.
> 
> These should be a part of the changelog.
> 

I added this to the [0/N]:

: Benefits include:
: 
: 1. Future clients can call page_ext_data directly instead of defining
:    a new function like get_page_owner to get the data.
: 
: 2. There is no change to clients if the layout of page_ext data changes.

Mike, what is your position on this patchset now?

Thanks.

Re: [PATCH 0/3] add page_ext_data to get client data in page_ext

[Mike Rapoport]

On Fri, Aug 11, 2023 at 02:39:29PM -0700, Andrew Morton wrote:
> On Thu, 20 Jul 2023 08:37:36 +0300 Mike Rapoport <rppt@kernel.org> wrote:
> 
> > Hi,
> > 
> > On Thu, Jul 20, 2023 at 10:38:39AM +0800, Kemeng Shi wrote:
> > > 
> > > on 7/19/2023 5:44 PM, Mike Rapoport wrote:
> > > > On Tue, Jul 18, 2023 at 10:58:09PM +0800, Kemeng Shi wrote:
> > > >> Current client get data from page_ext by adding offset which is auto
> > > >> generated in page_ext core and expose the data layout design insdie
> > > >> page_ext core. This series adds a page_ext_data to hide offset from
> > > >> client. Thanks!
> > > > 
> > > > Implementers of page_ext_operations are anyway intimately related to
> > > > page_ext, so I'm not convinced this has any value.
> > > >  
> > > Hi Mike, thanks for reply. I thinks page_ext_operations can be futher splited
> > > into public part which used by client to simply register and private part which
> > > only page_ext core cares and should not be accessed by client directly
> > > to reduce decoupling.
> > 
> > It would be easier to justify changes in this series if they were a part of
> > the refactoring you describe here.
> > 
> > > This series makes offset to be private which client
> > > doesn't really care to hide data layout inside page_ext core from client.
> > > There are some concrete gains I can list for now:
> > > 1. Future client cound call page_ext_data directly instead of define a
> > > new function like get_page_owner to get it's data.
> > > 2. No change to client if layout of page_ext data change.
> > 
> > These should be a part of the changelog.
> > 
> 
> I added this to the [0/N]:
> 
> : Benefits include:
> : 
> : 1. Future clients can call page_ext_data directly instead of defining
> :    a new function like get_page_owner to get the data.
> : 
> : 2. There is no change to clients if the layout of page_ext data changes.
> 
> Mike, what is your position on this patchset now?

I'm fine with it, so if it's not too much hassle to add it now

Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>

 
> Thanks.

-- 
Sincerely yours,
Mike.