| Message ID | 20230718145812.1991717-1-shikemeng@huaweicloud.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add page_ext_data to get client data in page_ext | expand |
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 > >
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!
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 >
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 >> >
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.
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.