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
use page_ext_data helper in page_table_check to avoid access offset
directly.
Signed-off-by: Kemeng Shi <[email protected]>
---
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
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.
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
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.
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
On Thu, 20 Jul 2023 08:37:36 +0300 Mike Rapoport <[email protected]> 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.
On Fri, Aug 11, 2023 at 02:39:29PM -0700, Andrew Morton wrote:
> On Thu, 20 Jul 2023 08:37:36 +0300 Mike Rapoport <[email protected]> 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) <[email protected]>
> Thanks.
--
Sincerely yours,
Mike.