* [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() @ 2022-09-28 9:56 Zhao Liu 2022-09-28 14:08 ` Wei Liu 2022-10-03 17:43 ` Fabio M. De Francesco 0 siblings, 2 replies; 9+ messages in thread From: Zhao Liu @ 2022-09-28 9:56 UTC (permalink / raw) To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel Cc: Ira Weiny, Fabio M . De Francesco, Zhenyu Wang, Zhao Liu, Dave Hansen From: Zhao Liu <zhao1.liu@intel.com> kmap() is being deprecated in favor of kmap_local_page()[1]. There are two main problems with kmap(): (1) It comes with an overhead as mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap's pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and are still valid. In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a single thread and is short live. So, in this case, it's safe to simply use kmap_local_page() to create mapping, and this avoids the wasted cost of kmap() for global synchronization. In addtion, the fuction hyperv_init() checks if kmap() fails by BUG_ON(). From the original discussion[2], the BUG_ON() here is just used to explicitly panic NULL pointer. So still keep the BUG_ON() in place to check if kmap_local_page() fails. Based on this consideration, memcpy_to_page() is not selected here but only kmap_local_page() is used. Therefore, replace kmap() with kmap_local_page() in hyperv/hv_init.c. [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com [2]: https://lore.kernel.org/lkml/20200915103710.cqmdvzh5lys4wsqo@liuwe-devbox-debian-v2/ Suggested-by: Dave Hansen <dave.hansen@intel.com> Suggested-by: Ira Weiny <ira.weiny@intel.com> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- Suggested by credits. Dave: Referred to his comments about whether kmap() can fail and the suggestion to keep BUG_ON() in place. Ira: Referred to his task documentation and review comments about keeping BUG_ON() for kmap_local_page(). Fabio: Stole some of his boiler plate commit message. --- arch/x86/hyperv/hv_init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c index 3de6d8b53367..72fe46eb183f 100644 --- a/arch/x86/hyperv/hv_init.c +++ b/arch/x86/hyperv/hv_init.c @@ -459,13 +459,13 @@ void __init hyperv_init(void) wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); pg = vmalloc_to_page(hv_hypercall_pg); - dst = kmap(pg); + dst = kmap_local_page(pg); src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE, MEMREMAP_WB); BUG_ON(!(src && dst)); memcpy(dst, src, HV_HYP_PAGE_SIZE); memunmap(src); - kunmap(pg); + kunmap_local(dst); } else { hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg); wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-09-28 9:56 [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() Zhao Liu @ 2022-09-28 14:08 ` Wei Liu 2022-10-03 8:50 ` Wei Liu 2022-10-03 17:43 ` Fabio M. De Francesco 1 sibling, 1 reply; 9+ messages in thread From: Wei Liu @ 2022-09-28 14:08 UTC (permalink / raw) To: Zhao Liu Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Ira Weiny, Fabio M . De Francesco, Zhenyu Wang, Zhao Liu, Dave Hansen On Wed, Sep 28, 2022 at 05:56:40PM +0800, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > There are two main problems with kmap(): (1) It comes with an overhead as > mapping space is restricted and protected by a global lock for > synchronization and (2) it also requires global TLB invalidation when the > kmap's pool wraps and it might block when the mapping space is fully > utilized until a slot becomes available. > > With kmap_local_page() the mappings are per thread, CPU local, can take > page faults, and can be called from any context (including interrupts). > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > the tasks can be preempted and, when they are scheduled to run again, the > kernel virtual addresses are restored and are still valid. > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > single thread and is short live. So, in this case, it's safe to simply use > kmap_local_page() to create mapping, and this avoids the wasted cost of > kmap() for global synchronization. > The kmap call in that function is not performance critical in any way, and at this point in the initialization process I don't expect there to be any contention, so the downside of kmap is not really a concern here. That being said, kmap getting deprecated is a good enough reason to switch to kmap_local_page. And I appreciate this well-written, well-reasoned commit message. I will apply it to hyperv-next later -- I doubt people will object to this change, but just in case. Thanks, Wei. > In addtion, the fuction hyperv_init() checks if kmap() fails by BUG_ON(). > From the original discussion[2], the BUG_ON() here is just used to > explicitly panic NULL pointer. So still keep the BUG_ON() in place to check > if kmap_local_page() fails. Based on this consideration, memcpy_to_page() > is not selected here but only kmap_local_page() is used. > > Therefore, replace kmap() with kmap_local_page() in hyperv/hv_init.c. > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com > [2]: https://lore.kernel.org/lkml/20200915103710.cqmdvzh5lys4wsqo@liuwe-devbox-debian-v2/ > > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Suggested-by: Ira Weiny <ira.weiny@intel.com> > Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > Suggested by credits. > Dave: Referred to his comments about whether kmap() can fail and the > suggestion to keep BUG_ON() in place. > Ira: Referred to his task documentation and review comments about > keeping BUG_ON() for kmap_local_page(). > Fabio: Stole some of his boiler plate commit message. > --- > arch/x86/hyperv/hv_init.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c > index 3de6d8b53367..72fe46eb183f 100644 > --- a/arch/x86/hyperv/hv_init.c > +++ b/arch/x86/hyperv/hv_init.c > @@ -459,13 +459,13 @@ void __init hyperv_init(void) > wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > > pg = vmalloc_to_page(hv_hypercall_pg); > - dst = kmap(pg); > + dst = kmap_local_page(pg); > src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE, > MEMREMAP_WB); > BUG_ON(!(src && dst)); > memcpy(dst, src, HV_HYP_PAGE_SIZE); > memunmap(src); > - kunmap(pg); > + kunmap_local(dst); > } else { > hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg); > wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-09-28 14:08 ` Wei Liu @ 2022-10-03 8:50 ` Wei Liu 2022-10-06 14:21 ` Zhao Liu 0 siblings, 1 reply; 9+ messages in thread From: Wei Liu @ 2022-10-03 8:50 UTC (permalink / raw) To: Zhao Liu Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Ira Weiny, Fabio M . De Francesco, Zhenyu Wang, Zhao Liu, Dave Hansen On Wed, Sep 28, 2022 at 02:08:40PM +0000, Wei Liu wrote: > On Wed, Sep 28, 2022 at 05:56:40PM +0800, Zhao Liu wrote: > > From: Zhao Liu <zhao1.liu@intel.com> > > > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > > > There are two main problems with kmap(): (1) It comes with an overhead as > > mapping space is restricted and protected by a global lock for > > synchronization and (2) it also requires global TLB invalidation when the > > kmap's pool wraps and it might block when the mapping space is fully > > utilized until a slot becomes available. > > > > With kmap_local_page() the mappings are per thread, CPU local, can take > > page faults, and can be called from any context (including interrupts). > > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > > the tasks can be preempted and, when they are scheduled to run again, the > > kernel virtual addresses are restored and are still valid. > > > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > > single thread and is short live. So, in this case, it's safe to simply use > > kmap_local_page() to create mapping, and this avoids the wasted cost of > > kmap() for global synchronization. > > > > The kmap call in that function is not performance critical in any way, > and at this point in the initialization process I don't expect there to > be any contention, so the downside of kmap is not really a concern here. > > That being said, kmap getting deprecated is a good enough reason to > switch to kmap_local_page. And I appreciate this well-written, > well-reasoned commit message. > > I will apply it to hyperv-next later -- I doubt people will object to > this change, but just in case. Applied to hyperv-next. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-10-03 8:50 ` Wei Liu @ 2022-10-06 14:21 ` Zhao Liu 2022-10-09 20:46 ` Wei Liu 0 siblings, 1 reply; 9+ messages in thread From: Zhao Liu @ 2022-10-06 14:21 UTC (permalink / raw) To: Wei Liu Cc: Ira Weiny, Fabio M . De Francesco, K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Zhenyu Wang, Zhao Liu On Mon, Oct 03, 2022 at 08:50:16AM +0000, Wei Liu wrote: > Date: Mon, 3 Oct 2022 08:50:16 +0000 > From: Wei Liu <wei.liu@kernel.org> > Subject: Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() > > On Wed, Sep 28, 2022 at 02:08:40PM +0000, Wei Liu wrote: > > On Wed, Sep 28, 2022 at 05:56:40PM +0800, Zhao Liu wrote: > > > From: Zhao Liu <zhao1.liu@intel.com> > > > > > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > > > > > There are two main problems with kmap(): (1) It comes with an overhead as > > > mapping space is restricted and protected by a global lock for > > > synchronization and (2) it also requires global TLB invalidation when the > > > kmap's pool wraps and it might block when the mapping space is fully > > > utilized until a slot becomes available. > > > > > > With kmap_local_page() the mappings are per thread, CPU local, can take > > > page faults, and can be called from any context (including interrupts). > > > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > > > the tasks can be preempted and, when they are scheduled to run again, the > > > kernel virtual addresses are restored and are still valid. > > > > > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > > > single thread and is short live. So, in this case, it's safe to simply use > > > kmap_local_page() to create mapping, and this avoids the wasted cost of > > > kmap() for global synchronization. > > > > > > > The kmap call in that function is not performance critical in any way, > > and at this point in the initialization process I don't expect there to > > be any contention, so the downside of kmap is not really a concern here. > > > > That being said, kmap getting deprecated is a good enough reason to > > switch to kmap_local_page. And I appreciate this well-written, > > well-reasoned commit message. > > > > I will apply it to hyperv-next later -- I doubt people will object to > > this change, but just in case. > > Applied to hyperv-next. Thanks. Sorry Wei, based on Ira and Fabio's comments, do you agree me to send a follow on patch to remove that BUG_ON()? Or send the v2 patch? Thanks, Zhao ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-10-06 14:21 ` Zhao Liu @ 2022-10-09 20:46 ` Wei Liu 2022-10-10 12:06 ` Zhao Liu 0 siblings, 1 reply; 9+ messages in thread From: Wei Liu @ 2022-10-09 20:46 UTC (permalink / raw) To: Zhao Liu Cc: Wei Liu, Ira Weiny, Fabio M . De Francesco, K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Zhenyu Wang, Zhao Liu On Thu, Oct 06, 2022 at 10:21:43PM +0800, Zhao Liu wrote: > On Mon, Oct 03, 2022 at 08:50:16AM +0000, Wei Liu wrote: > > Date: Mon, 3 Oct 2022 08:50:16 +0000 > > From: Wei Liu <wei.liu@kernel.org> > > Subject: Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() > > > > On Wed, Sep 28, 2022 at 02:08:40PM +0000, Wei Liu wrote: > > > On Wed, Sep 28, 2022 at 05:56:40PM +0800, Zhao Liu wrote: > > > > From: Zhao Liu <zhao1.liu@intel.com> > > > > > > > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > > > > > > > There are two main problems with kmap(): (1) It comes with an overhead as > > > > mapping space is restricted and protected by a global lock for > > > > synchronization and (2) it also requires global TLB invalidation when the > > > > kmap's pool wraps and it might block when the mapping space is fully > > > > utilized until a slot becomes available. > > > > > > > > With kmap_local_page() the mappings are per thread, CPU local, can take > > > > page faults, and can be called from any context (including interrupts). > > > > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > > > > the tasks can be preempted and, when they are scheduled to run again, the > > > > kernel virtual addresses are restored and are still valid. > > > > > > > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > > > > single thread and is short live. So, in this case, it's safe to simply use > > > > kmap_local_page() to create mapping, and this avoids the wasted cost of > > > > kmap() for global synchronization. > > > > > > > > > > The kmap call in that function is not performance critical in any way, > > > and at this point in the initialization process I don't expect there to > > > be any contention, so the downside of kmap is not really a concern here. > > > > > > That being said, kmap getting deprecated is a good enough reason to > > > switch to kmap_local_page. And I appreciate this well-written, > > > well-reasoned commit message. > > > > > > I will apply it to hyperv-next later -- I doubt people will object to > > > this change, but just in case. > > > > Applied to hyperv-next. Thanks. > > Sorry Wei, based on Ira and Fabio's comments, do you agree me to send a > follow on patch to remove that BUG_ON()? Or send the v2 patch? > I just sent a PR to Linus. That PR includes your v1 patch. While the code can be improved, BUG_ON that does nothing is harmless. You can send a follow-up patch to remove the BUG_ON. Thanks, Wei. > Thanks, > Zhao ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-10-09 20:46 ` Wei Liu @ 2022-10-10 12:06 ` Zhao Liu 0 siblings, 0 replies; 9+ messages in thread From: Zhao Liu @ 2022-10-10 12:06 UTC (permalink / raw) To: Wei Liu Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Ira Weiny, Fabio M . De Francesco, Zhenyu Wang, Zhao Liu On Sun, Oct 09, 2022 at 08:46:33PM +0000, Wei Liu wrote: > Date: Sun, 9 Oct 2022 20:46:33 +0000 > From: Wei Liu <wei.liu@kernel.org> > Subject: Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() > > On Thu, Oct 06, 2022 at 10:21:43PM +0800, Zhao Liu wrote: > > On Mon, Oct 03, 2022 at 08:50:16AM +0000, Wei Liu wrote: > > > Date: Mon, 3 Oct 2022 08:50:16 +0000 > > > From: Wei Liu <wei.liu@kernel.org> > > > Subject: Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() > > > > > > On Wed, Sep 28, 2022 at 02:08:40PM +0000, Wei Liu wrote: > > > > On Wed, Sep 28, 2022 at 05:56:40PM +0800, Zhao Liu wrote: > > > > > From: Zhao Liu <zhao1.liu@intel.com> > > > > > > > > > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > > > > > > > > > There are two main problems with kmap(): (1) It comes with an overhead as > > > > > mapping space is restricted and protected by a global lock for > > > > > synchronization and (2) it also requires global TLB invalidation when the > > > > > kmap's pool wraps and it might block when the mapping space is fully > > > > > utilized until a slot becomes available. > > > > > > > > > > With kmap_local_page() the mappings are per thread, CPU local, can take > > > > > page faults, and can be called from any context (including interrupts). > > > > > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > > > > > the tasks can be preempted and, when they are scheduled to run again, the > > > > > kernel virtual addresses are restored and are still valid. > > > > > > > > > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > > > > > single thread and is short live. So, in this case, it's safe to simply use > > > > > kmap_local_page() to create mapping, and this avoids the wasted cost of > > > > > kmap() for global synchronization. > > > > > > > > > > > > > The kmap call in that function is not performance critical in any way, > > > > and at this point in the initialization process I don't expect there to > > > > be any contention, so the downside of kmap is not really a concern here. > > > > > > > > That being said, kmap getting deprecated is a good enough reason to > > > > switch to kmap_local_page. And I appreciate this well-written, > > > > well-reasoned commit message. > > > > > > > > I will apply it to hyperv-next later -- I doubt people will object to > > > > this change, but just in case. > > > > > > Applied to hyperv-next. Thanks. > > > > Sorry Wei, based on Ira and Fabio's comments, do you agree me to send a > > follow on patch to remove that BUG_ON()? Or send the v2 patch? > > > > I just sent a PR to Linus. That PR includes your v1 patch. > > While the code can be improved, BUG_ON that does nothing is harmless. > You can send a follow-up patch to remove the BUG_ON. Thanks Wei! I'll do that. Regards, Zhao > > Thanks, > Wei. > > > Thanks, > > Zhao ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-09-28 9:56 [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() Zhao Liu 2022-09-28 14:08 ` Wei Liu @ 2022-10-03 17:43 ` Fabio M. De Francesco 2022-10-03 20:24 ` Ira Weiny 1 sibling, 1 reply; 9+ messages in thread From: Fabio M. De Francesco @ 2022-10-03 17:43 UTC (permalink / raw) To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Zhao Liu, Ira Weiny Cc: Zhenyu Wang, Zhao Liu, Dave Hansen On Wednesday, September 28, 2022 11:56:40 AM CEST Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > There are two main problems with kmap(): (1) It comes with an overhead as > mapping space is restricted and protected by a global lock for > synchronization and (2) it also requires global TLB invalidation when the > kmap's pool wraps and it might block when the mapping space is fully > utilized until a slot becomes available. > > With kmap_local_page() the mappings are per thread, CPU local, can take > page faults, and can be called from any context (including interrupts). > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > the tasks can be preempted and, when they are scheduled to run again, the > kernel virtual addresses are restored and are still valid. > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > single thread and is short live. So, in this case, it's safe to simply use > kmap_local_page() to create mapping, and this avoids the wasted cost of > kmap() for global synchronization. > > In addtion, the fuction hyperv_init() checks if kmap() fails by BUG_ON(). > From the original discussion[2], the BUG_ON() here is just used to > explicitly panic NULL pointer. So still keep the BUG_ON() in place to check > if kmap_local_page() fails. How might kmap_local_page() return invalid kernel addresses? I think that, if this function returns, the pointer is always a valid kernel address. Am I missing something? > Based on this consideration, memcpy_to_page() > is not selected here but only kmap_local_page() is used. I can't agree with you, if the premises are that kmap_local_page() might provide invalid addresses. Thanks, Fabio > Therefore, replace kmap() with kmap_local_page() in hyperv/hv_init.c. > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com > [2]: https://lore.kernel.org/lkml/20200915103710.cqmdvzh5lys4wsqo@liuwe-devbox-debian-v2/ > > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Suggested-by: Ira Weiny <ira.weiny@intel.com> > Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > Suggested by credits. > Dave: Referred to his comments about whether kmap() can fail and the > suggestion to keep BUG_ON() in place. > Ira: Referred to his task documentation and review comments about > keeping BUG_ON() for kmap_local_page(). > Fabio: Stole some of his boiler plate commit message. > --- > arch/x86/hyperv/hv_init.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-10-03 17:43 ` Fabio M. De Francesco @ 2022-10-03 20:24 ` Ira Weiny 2022-10-06 14:13 ` Zhao Liu 0 siblings, 1 reply; 9+ messages in thread From: Ira Weiny @ 2022-10-03 20:24 UTC (permalink / raw) To: Fabio M. De Francesco Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Zhao Liu, Zhenyu Wang, Zhao Liu, Dave Hansen On Mon, Oct 03, 2022 at 07:43:49PM +0200, Fabio M. De Francesco wrote: > On Wednesday, September 28, 2022 11:56:40 AM CEST Zhao Liu wrote: > > From: Zhao Liu <zhao1.liu@intel.com> > > > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > > > There are two main problems with kmap(): (1) It comes with an overhead as > > mapping space is restricted and protected by a global lock for > > synchronization and (2) it also requires global TLB invalidation when the > > kmap's pool wraps and it might block when the mapping space is fully > > utilized until a slot becomes available. > > > > With kmap_local_page() the mappings are per thread, CPU local, can take > > page faults, and can be called from any context (including interrupts). > > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > > the tasks can be preempted and, when they are scheduled to run again, the > > kernel virtual addresses are restored and are still valid. > > > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > > single thread and is short live. So, in this case, it's safe to simply use > > kmap_local_page() to create mapping, and this avoids the wasted cost of > > kmap() for global synchronization. > > > > In addtion, the fuction hyperv_init() checks if kmap() fails by BUG_ON(). > > From the original discussion[2], the BUG_ON() here is just used to > > explicitly panic NULL pointer. So still keep the BUG_ON() in place to check > > if kmap_local_page() fails. > > How might kmap_local_page() return invalid kernel addresses? > > I think that, if this function returns, the pointer is always a valid kernel > address. Am I missing something? I think this is my mistake. I did not realize what Zhao was asking me the other day and generally said to leave BUG_ON's alone and not change things. In this case kmap_local_page() will not return NULL. It will BUG on its own if it fails. > > > Based on this consideration, memcpy_to_page() > > is not selected here but only kmap_local_page() is used. > > I can't agree with you, if the premises are that kmap_local_page() might > provide invalid addresses. I'm ok with the patch as is. But Fabio is correct and it may be worth removing the check in a follow on patch. Ira > Thanks, > > Fabio > > > Therefore, replace kmap() with kmap_local_page() in hyperv/hv_init.c. > > > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com > > [2]: https://lore.kernel.org/lkml/20200915103710.cqmdvzh5lys4wsqo@liuwe-devbox-debian-v2/ > > > > Suggested-by: Dave Hansen <dave.hansen@intel.com> > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > > Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > > > --- > > Suggested by credits. > > Dave: Referred to his comments about whether kmap() can fail and > the > > suggestion to keep BUG_ON() in place. > > Ira: Referred to his task documentation and review comments about > > keeping BUG_ON() for kmap_local_page(). > > Fabio: Stole some of his boiler plate commit message. > > --- > > arch/x86/hyperv/hv_init.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() 2022-10-03 20:24 ` Ira Weiny @ 2022-10-06 14:13 ` Zhao Liu 0 siblings, 0 replies; 9+ messages in thread From: Zhao Liu @ 2022-10-06 14:13 UTC (permalink / raw) To: Ira Weiny, Fabio M . De Francesco, Wei Liu Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv, linux-kernel, Zhenyu Wang, Zhao Liu On Mon, Oct 03, 2022 at 01:24:51PM -0700, Ira Weiny wrote: > Date: Mon, 3 Oct 2022 13:24:51 -0700 > From: Ira Weiny <ira.weiny@intel.com> > Subject: Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() > > On Mon, Oct 03, 2022 at 07:43:49PM +0200, Fabio M. De Francesco wrote: > > On Wednesday, September 28, 2022 11:56:40 AM CEST Zhao Liu wrote: > > > From: Zhao Liu <zhao1.liu@intel.com> > > > > > > kmap() is being deprecated in favor of kmap_local_page()[1]. > > > > > > There are two main problems with kmap(): (1) It comes with an overhead as > > > mapping space is restricted and protected by a global lock for > > > synchronization and (2) it also requires global TLB invalidation when the > > > kmap's pool wraps and it might block when the mapping space is fully > > > utilized until a slot becomes available. > > > > > > With kmap_local_page() the mappings are per thread, CPU local, can take > > > page faults, and can be called from any context (including interrupts). > > > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > > > the tasks can be preempted and, when they are scheduled to run again, the > > > kernel virtual addresses are restored and are still valid. > > > > > > In the fuction hyperv_init() of hyperv/hv_init.c, the mapping is used in a > > > single thread and is short live. So, in this case, it's safe to simply use > > > kmap_local_page() to create mapping, and this avoids the wasted cost of > > > kmap() for global synchronization. > > > > > > In addtion, the fuction hyperv_init() checks if kmap() fails by BUG_ON(). > > > From the original discussion[2], the BUG_ON() here is just used to > > > explicitly panic NULL pointer. So still keep the BUG_ON() in place to check > > > if kmap_local_page() fails. > > > > How might kmap_local_page() return invalid kernel addresses? > > > > I think that, if this function returns, the pointer is always a valid kernel > > address. Am I missing something? > > I think this is my mistake. I did not realize what Zhao was asking me the > other day and generally said to leave BUG_ON's alone and not change things. Thanks Fabio and Ira! This is my negligence, my consideration is still not thorough enough. I'll remove this BUG_ON(). > > In this case kmap_local_page() will not return NULL. It will BUG on its own if > it fails. > > > > > > Based on this consideration, memcpy_to_page() > > > is not selected here but only kmap_local_page() is used. > > > > I can't agree with you, if the premises are that kmap_local_page() might > > provide invalid addresses. > > I'm ok with the patch as is. But Fabio is correct and it may be worth removing > the check in a follow on patch. > Ira > > > Thanks, > > > > Fabio > > > > > Therefore, replace kmap() with kmap_local_page() in hyperv/hv_init.c. > > > > > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com > > > [2]: https://lore.kernel.org/lkml/20200915103710.cqmdvzh5lys4wsqo@liuwe-devbox-debian-v2/ > > > > > > Suggested-by: Dave Hansen <dave.hansen@intel.com> > > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > > > Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > > > > > --- > > > Suggested by credits. > > > Dave: Referred to his comments about whether kmap() can fail and > > the > > > suggestion to keep BUG_ON() in place. > > > Ira: Referred to his task documentation and review comments about > > > keeping BUG_ON() for kmap_local_page(). > > > Fabio: Stole some of his boiler plate commit message. > > > --- > > > arch/x86/hyperv/hv_init.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-10-10 12:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-28 9:56 [PATCH] x86/hyperv: Replace kmap() with kmap_local_page() Zhao Liu 2022-09-28 14:08 ` Wei Liu 2022-10-03 8:50 ` Wei Liu 2022-10-06 14:21 ` Zhao Liu 2022-10-09 20:46 ` Wei Liu 2022-10-10 12:06 ` Zhao Liu 2022-10-03 17:43 ` Fabio M. De Francesco 2022-10-03 20:24 ` Ira Weiny 2022-10-06 14:13 ` Zhao Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).