[PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[Zhao Liu]

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

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[Wei Liu]

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
> 

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[Wei Liu]

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.

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[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

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[Wei 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

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[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

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[Fabio M. De Francesco]

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(-)
> 

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[Ira Weiny]

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(-)
> > 
> 
> 
> 

Re: [PATCH] x86/hyperv: Replace kmap() with kmap_local_page()

[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(-)
> > > 
> > 
> > 
> >