Message ID | 20200408115926.1467567-11-hch@lst.de |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Series | [01/28] x86/hyperv: use vmalloc_exec for the hypercall page | expand |
Hi, On 4/8/20 4:59 AM, Christoph Hellwig wrote: > diff --git a/mm/Kconfig b/mm/Kconfig > index 36949a9425b8..614cc786b519 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -702,7 +702,7 @@ config ZSMALLOC > > config ZSMALLOC_PGTABLE_MAPPING > bool "Use page table mapping to access object in zsmalloc" > - depends on ZSMALLOC > + depends on ZSMALLOC=y It's a bool so this shouldn't matter... not needed. > help > By default, zsmalloc uses a copy-based object mapping method to > access allocations that span two pages. However, if a particular
On Wed, Apr 08, 2020 at 08:01:00AM -0700, Randy Dunlap wrote: > Hi, > > On 4/8/20 4:59 AM, Christoph Hellwig wrote: > > diff --git a/mm/Kconfig b/mm/Kconfig > > index 36949a9425b8..614cc786b519 100644 > > --- a/mm/Kconfig > > +++ b/mm/Kconfig > > @@ -702,7 +702,7 @@ config ZSMALLOC > > > > config ZSMALLOC_PGTABLE_MAPPING > > bool "Use page table mapping to access object in zsmalloc" > > - depends on ZSMALLOC > > + depends on ZSMALLOC=y > > It's a bool so this shouldn't matter... not needed. My mm/Kconfig has: config ZSMALLOC tristate "Memory allocator for compressed pages" depends on MMU which I think means it can be modular, no?
On Wed, Apr 08, 2020 at 05:12:03PM +0200, Peter Zijlstra wrote: > On Wed, Apr 08, 2020 at 08:01:00AM -0700, Randy Dunlap wrote: > > Hi, > > > > On 4/8/20 4:59 AM, Christoph Hellwig wrote: > > > diff --git a/mm/Kconfig b/mm/Kconfig > > > index 36949a9425b8..614cc786b519 100644 > > > --- a/mm/Kconfig > > > +++ b/mm/Kconfig > > > @@ -702,7 +702,7 @@ config ZSMALLOC > > > > > > config ZSMALLOC_PGTABLE_MAPPING > > > bool "Use page table mapping to access object in zsmalloc" > > > - depends on ZSMALLOC > > > + depends on ZSMALLOC=y > > > > It's a bool so this shouldn't matter... not needed. > > My mm/Kconfig has: > > config ZSMALLOC > tristate "Memory allocator for compressed pages" > depends on MMU > > which I think means it can be modular, no? Randy means that ZSMALLOC_PGTABLE_MAPPING is a bool, so I think hch's patch is wrong ... if ZSMALLOC is 'm' then ZSMALLOC_PGTABLE_MAPPING would become 'n' instead of 'y'.
On Wed, Apr 08, 2020 at 08:15:19AM -0700, Matthew Wilcox wrote: > > > > config ZSMALLOC_PGTABLE_MAPPING > > > > bool "Use page table mapping to access object in zsmalloc" > > > > - depends on ZSMALLOC > > > > + depends on ZSMALLOC=y > > > > > > It's a bool so this shouldn't matter... not needed. > > > > My mm/Kconfig has: > > > > config ZSMALLOC > > tristate "Memory allocator for compressed pages" > > depends on MMU > > > > which I think means it can be modular, no? > > Randy means that ZSMALLOC_PGTABLE_MAPPING is a bool, so I think hch's patch > is wrong ... if ZSMALLOC is 'm' then ZSMALLOC_PGTABLE_MAPPING would become > 'n' instead of 'y'. In Linus' tree you can select PGTABLE_MAPPING=y with ZSMALLOC=m, and that fits my understanding of the kbuild language. With this patch I can't anymore.
On 4/8/20 8:15 AM, Matthew Wilcox wrote: > On Wed, Apr 08, 2020 at 05:12:03PM +0200, Peter Zijlstra wrote: >> On Wed, Apr 08, 2020 at 08:01:00AM -0700, Randy Dunlap wrote: >>> Hi, >>> >>> On 4/8/20 4:59 AM, Christoph Hellwig wrote: >>>> diff --git a/mm/Kconfig b/mm/Kconfig >>>> index 36949a9425b8..614cc786b519 100644 >>>> --- a/mm/Kconfig >>>> +++ b/mm/Kconfig >>>> @@ -702,7 +702,7 @@ config ZSMALLOC >>>> >>>> config ZSMALLOC_PGTABLE_MAPPING >>>> bool "Use page table mapping to access object in zsmalloc" >>>> - depends on ZSMALLOC >>>> + depends on ZSMALLOC=y >>> >>> It's a bool so this shouldn't matter... not needed. >> >> My mm/Kconfig has: >> >> config ZSMALLOC >> tristate "Memory allocator for compressed pages" >> depends on MMU >> >> which I think means it can be modular, no? ack. I misread it. > Randy means that ZSMALLOC_PGTABLE_MAPPING is a bool, so I think hch's patch > is wrong ... if ZSMALLOC is 'm' then ZSMALLOC_PGTABLE_MAPPING would become > 'n' instead of 'y'. sigh, I wish that I had meant that. :) thanks.
On 4/8/20 8:36 AM, Christoph Hellwig wrote: > On Wed, Apr 08, 2020 at 08:15:19AM -0700, Matthew Wilcox wrote: >>>>> config ZSMALLOC_PGTABLE_MAPPING >>>>> bool "Use page table mapping to access object in zsmalloc" >>>>> - depends on ZSMALLOC >>>>> + depends on ZSMALLOC=y >>>> >>>> It's a bool so this shouldn't matter... not needed. >>> >>> My mm/Kconfig has: >>> >>> config ZSMALLOC >>> tristate "Memory allocator for compressed pages" >>> depends on MMU >>> >>> which I think means it can be modular, no? >> >> Randy means that ZSMALLOC_PGTABLE_MAPPING is a bool, so I think hch's patch >> is wrong ... if ZSMALLOC is 'm' then ZSMALLOC_PGTABLE_MAPPING would become >> 'n' instead of 'y'. > > In Linus' tree you can select PGTABLE_MAPPING=y with ZSMALLOC=m, > and that fits my understanding of the kbuild language. With this > patch I can't anymore. > Makes sense. thanks.
On Wed, Apr 08, 2020 at 01:59:08PM +0200, Christoph Hellwig wrote: > This allows to unexport map_vm_area and unmap_kernel_range, which are > rather deep internal and should not be available to modules. Even though I don't know how many usecase we have using zsmalloc as module(I heard only once by dumb reason), it could affect existing users. Thus, please include concrete explanation in the patch to justify when the complain occurs. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > mm/Kconfig | 2 +- > mm/vmalloc.c | 2 -- > 2 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/Kconfig b/mm/Kconfig > index 36949a9425b8..614cc786b519 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -702,7 +702,7 @@ config ZSMALLOC > > config ZSMALLOC_PGTABLE_MAPPING > bool "Use page table mapping to access object in zsmalloc" > - depends on ZSMALLOC > + depends on ZSMALLOC=y > help > By default, zsmalloc uses a copy-based object mapping method to > access allocations that span two pages. However, if a particular > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 3375f9508ef6..9183fc0d365a 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2046,7 +2046,6 @@ void unmap_kernel_range(unsigned long addr, unsigned long size) > vunmap_page_range(addr, end); > flush_tlb_kernel_range(addr, end); > } > -EXPORT_SYMBOL_GPL(unmap_kernel_range); > > int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page **pages) > { > @@ -2058,7 +2057,6 @@ int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page **pages) > > return err > 0 ? 0 : err; > } > -EXPORT_SYMBOL_GPL(map_vm_area); > > static inline void setup_vmalloc_vm_locked(struct vm_struct *vm, > struct vmap_area *va, unsigned long flags, const void *caller) > -- > 2.25.1 >
On Thu, Apr 09, 2020 at 09:08:26AM -0700, Minchan Kim wrote: > On Wed, Apr 08, 2020 at 01:59:08PM +0200, Christoph Hellwig wrote: > > This allows to unexport map_vm_area and unmap_kernel_range, which are > > rather deep internal and should not be available to modules. > > Even though I don't know how many usecase we have using zsmalloc as > module(I heard only once by dumb reason), it could affect existing > users. Thus, please include concrete explanation in the patch to > justify when the complain occurs. The justification is 'we can unexport functions that have no sane reason of being exported in the first place'. The Changelog pretty much says that.
On Thu, Apr 09, 2020 at 06:50:30PM +0200, Peter Zijlstra wrote: > On Thu, Apr 09, 2020 at 09:08:26AM -0700, Minchan Kim wrote: > > On Wed, Apr 08, 2020 at 01:59:08PM +0200, Christoph Hellwig wrote: > > > This allows to unexport map_vm_area and unmap_kernel_range, which are > > > rather deep internal and should not be available to modules. > > > > Even though I don't know how many usecase we have using zsmalloc as > > module(I heard only once by dumb reason), it could affect existing > > users. Thus, please include concrete explanation in the patch to > > justify when the complain occurs. > > The justification is 'we can unexport functions that have no sane reason > of being exported in the first place'. > > The Changelog pretty much says that. Okay, I hope there is no affected user since this patch. If there are someone, they need to provide sane reason why they want to have zsmalloc as module. Acked-by: Minchan Kim <minchan@kernel.org>
On (20/04/09 10:08), Minchan Kim wrote: > > > Even though I don't know how many usecase we have using zsmalloc as > > > module(I heard only once by dumb reason), it could affect existing > > > users. Thus, please include concrete explanation in the patch to > > > justify when the complain occurs. > > > > The justification is 'we can unexport functions that have no sane reason > > of being exported in the first place'. > > > > The Changelog pretty much says that. > > Okay, I hope there is no affected user since this patch. > If there are someone, they need to provide sane reason why they want > to have zsmalloc as module. I'm one of those who use zsmalloc as a module - mainly because I use zram as a compressing general purpose block device, not as a swap device. I create zram0, mkfs, mount, checkout and compile code, once done - umount, rmmod. This reduces the number of writes to SSD. Some people use tmpfs, but zram device(-s) can be much larger in size. That's a niche use case and I'm not against the patch. -ss
Hi Sergey, On Fri, Apr 10, 2020 at 11:38:45AM +0900, Sergey Senozhatsky wrote: > On (20/04/09 10:08), Minchan Kim wrote: > > > > Even though I don't know how many usecase we have using zsmalloc as > > > > module(I heard only once by dumb reason), it could affect existing > > > > users. Thus, please include concrete explanation in the patch to > > > > justify when the complain occurs. > > > > > > The justification is 'we can unexport functions that have no sane reason > > > of being exported in the first place'. > > > > > > The Changelog pretty much says that. > > > > Okay, I hope there is no affected user since this patch. > > If there are someone, they need to provide sane reason why they want > > to have zsmalloc as module. > > I'm one of those who use zsmalloc as a module - mainly because I use zram > as a compressing general purpose block device, not as a swap device. > I create zram0, mkfs, mount, checkout and compile code, once done - > umount, rmmod. This reduces the number of writes to SSD. Some people use > tmpfs, but zram device(-s) can be much larger in size. That's a niche use > case and I'm not against the patch. It doesn't mean we couldn't use zsmalloc as module any longer. It means we couldn't use zsmalloc as module with pgtable mapping whcih was little bit faster on microbenchmark in some architecutre(However, I usually temped to remove it since it had several problems). However, we could still use zsmalloc as module as copy way instead of pgtable mapping. Thus, if someone really want to rollback the feature, they should provide reasonable reason why it doesn't work for them. "A little fast" wouldn't be enough to exports deep internal to the module. Thanks.
Hi Minchan, On Fri, Apr 10, 2020 at 04:11:36PM -0700, Minchan Kim wrote: > It doesn't mean we couldn't use zsmalloc as module any longer. It means > we couldn't use zsmalloc as module with pgtable mapping whcih was little > bit faster on microbenchmark in some architecutre(However, I usually temped > to remove it since it had several problems). However, we could still use > zsmalloc as module as copy way instead of pgtable mapping. Thus, if someone > really want to rollback the feature, they should provide reasonable reason > why it doesn't work for them. "A little fast" wouldn't be enough to exports > deep internal to the module. do you have any data how much faster it is on arm (and does that include arm64 as well)? Besides the exports which were my prime concern, zsmalloc with pgtable mappings also is the only user of map_kernel_range outside of vmalloc.c, if it really is another code base for tiny improvements we could mark map_kernel_range or in fact remove it entirely and open code it in the remaining callers. (unmap_kernel_range is a different story, it has a bunch of callers, and most look odd)
Hi Christoph, Sorry for the late. On Sat, Apr 11, 2020 at 09:20:52AM +0200, Christoph Hellwig wrote: > Hi Minchan, > > On Fri, Apr 10, 2020 at 04:11:36PM -0700, Minchan Kim wrote: > > It doesn't mean we couldn't use zsmalloc as module any longer. It means > > we couldn't use zsmalloc as module with pgtable mapping whcih was little > > bit faster on microbenchmark in some architecutre(However, I usually temped > > to remove it since it had several problems). However, we could still use > > zsmalloc as module as copy way instead of pgtable mapping. Thus, if someone > > really want to rollback the feature, they should provide reasonable reason > > why it doesn't work for them. "A little fast" wouldn't be enough to exports > > deep internal to the module. > > do you have any data how much faster it is on arm (and does that include > arm64 as well)? Besides the exports which were my prime concern, https://github.com/sjenning/zsmapbench I need to recall the memory. IIRC, it was almost 30% faster at that time in ARM so was not trivial at that time. However, it was story from several years ago. > zsmalloc with pgtable mappings also is the only user of map_kernel_range > outside of vmalloc.c, if it really is another code base for tiny > improvements we could mark map_kernel_range or in fact remove it entirely > and open code it in the remaining callers. I alsh have temped to remove it. Let me have time to revist it in this chance. Thanks.
diff --git a/mm/Kconfig b/mm/Kconfig index 36949a9425b8..614cc786b519 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -702,7 +702,7 @@ config ZSMALLOC config ZSMALLOC_PGTABLE_MAPPING bool "Use page table mapping to access object in zsmalloc" - depends on ZSMALLOC + depends on ZSMALLOC=y help By default, zsmalloc uses a copy-based object mapping method to access allocations that span two pages. However, if a particular diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 3375f9508ef6..9183fc0d365a 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2046,7 +2046,6 @@ void unmap_kernel_range(unsigned long addr, unsigned long size) vunmap_page_range(addr, end); flush_tlb_kernel_range(addr, end); } -EXPORT_SYMBOL_GPL(unmap_kernel_range); int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page **pages) { @@ -2058,7 +2057,6 @@ int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page **pages) return err > 0 ? 0 : err; } -EXPORT_SYMBOL_GPL(map_vm_area); static inline void setup_vmalloc_vm_locked(struct vm_struct *vm, struct vmap_area *va, unsigned long flags, const void *caller)
This allows to unexport map_vm_area and unmap_kernel_range, which are rather deep internal and should not be available to modules. Signed-off-by: Christoph Hellwig <hch@lst.de> --- mm/Kconfig | 2 +- mm/vmalloc.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)