[PATCH v2] lib: Replace kmap() with kmap_local_page()

[PATCH v2] lib: Replace kmap() with kmap_local_page()

[Sumitra Sharma]

kmap() has been deprecated in favor of the kmap_local_page()
due to high cost, restricted mapping space, the overhead of
a global lock for synchronization, and making the process
sleep in the absence of free slots.

kmap_local_page() is faster than kmap() and offers thread-local
and CPU-local mappings, take pagefaults in a local kmap region
and preserves preemption by saving the mappings of outgoing
tasks and restoring those of the incoming one during a context
switch.

The mappings are kept thread local in the functions
“dmirror_do_read” and “dmirror_do_write” in test_hmm.c

Therefore, replace kmap() with kmap_local_page() and use
mempcy_from/to_page() to avoid open coding kmap_local_page()
+ memcpy() + kunmap_local().

Remove the unused variable “tmp”.


Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---

Changes in v2: 
	- Change commit subject and description.
	- Remove unnecessary type casting (char *).


 lib/test_hmm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 67e6f83fe0f8..717dcb830127 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -368,16 +368,13 @@ static int dmirror_do_read(struct dmirror *dmirror, unsigned long start,
 	for (pfn = start >> PAGE_SHIFT; pfn < (end >> PAGE_SHIFT); pfn++) {
 		void *entry;
 		struct page *page;
-		void *tmp;
 
 		entry = xa_load(&dmirror->pt, pfn);
 		page = xa_untag_pointer(entry);
 		if (!page)
 			return -ENOENT;
 
-		tmp = kmap(page);
-		memcpy(ptr, tmp, PAGE_SIZE);
-		kunmap(page);
+		memcpy_from_page(ptr, page, 0, PAGE_SIZE);
 
 		ptr += PAGE_SIZE;
 		bounce->cpages++;
@@ -437,16 +434,13 @@ static int dmirror_do_write(struct dmirror *dmirror, unsigned long start,
 	for (pfn = start >> PAGE_SHIFT; pfn < (end >> PAGE_SHIFT); pfn++) {
 		void *entry;
 		struct page *page;
-		void *tmp;
 
 		entry = xa_load(&dmirror->pt, pfn);
 		page = xa_untag_pointer(entry);
 		if (!page || xa_pointer_tag(entry) != DPT_XA_TAG_WRITE)
 			return -ENOENT;
 
-		tmp = kmap(page);
-		memcpy(tmp, ptr, PAGE_SIZE);
-		kunmap(page);
+		memcpy_to_page(page, 0, ptr, PAGE_SIZE);
 
 		ptr += PAGE_SIZE;
 		bounce->cpages++;
-- 
2.25.1

Re: [PATCH v2] lib: Replace kmap() with kmap_local_page()

[Ira Weiny]

Sumitra Sharma wrote:
> kmap() has been deprecated in favor of the kmap_local_page()
> due to high cost, restricted mapping space, the overhead of
> a global lock for synchronization, and making the process
> sleep in the absence of free slots.
> 
> kmap_local_page() is faster than kmap() and offers thread-local
> and CPU-local mappings, take pagefaults in a local kmap region
> and preserves preemption by saving the mappings of outgoing
> tasks and restoring those of the incoming one during a context
> switch.
> 
> The mappings are kept thread local in the functions
> “dmirror_do_read” and “dmirror_do_write” in test_hmm.c
> 
> Therefore, replace kmap() with kmap_local_page() and use
> mempcy_from/to_page() to avoid open coding kmap_local_page()
> + memcpy() + kunmap_local().
> 
> Remove the unused variable “tmp”.
> 
> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>

LGTM

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH v2] lib: Replace kmap() with kmap_local_page()

[Fabio M. De Francesco]

On sabato 10 giugno 2023 19:57:12 CEST Sumitra Sharma wrote:
> kmap() has been deprecated in favor of the kmap_local_page()
> due to high cost, restricted mapping space, the overhead of
> a global lock for synchronization, and making the process
> sleep in the absence of free slots.
> 
> kmap_local_page() is faster than kmap() and offers thread-local
> and CPU-local mappings, take pagefaults in a local kmap region
> and preserves preemption by saving the mappings of outgoing
> tasks and restoring those of the incoming one during a context
> switch.
> 
> The mappings are kept thread local in the functions
> “dmirror_do_read” and “dmirror_do_write” in test_hmm.c
> 
> Therefore, replace kmap() with kmap_local_page() and use
> mempcy_from/to_page() to avoid open coding kmap_local_page()
> + memcpy() + kunmap_local().
> 
> Remove the unused variable “tmp”.
> 
> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

LGTM, so...

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

P.S.: The answer to an old question from you is that "LGTM" stands for "[It] 
Looks Good To Me". 

It's just a way to introduce the "Reviewed-by" tag itself. However, "LGTM" is 
not required, whereas the tag is required for a valid review and only the tag 
line will be added by maintainers in the log when your patch gets applied.  

While here... Please don't put unnecessary blank lines between the tags.They 
are not required and instead may worsen readability (obviously, I'm not 
requiring a new version for this).  

> 
> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> ---
> 
> Changes in v2:
> 	- Change commit subject and description.
> 	- Remove unnecessary type casting (char *).
> 
> 
>  lib/test_hmm.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 67e6f83fe0f8..717dcb830127 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -368,16 +368,13 @@ static int dmirror_do_read(struct dmirror *dmirror,
> unsigned long start, for (pfn = start >> PAGE_SHIFT; pfn < (end >>
> PAGE_SHIFT); pfn++) { void *entry;
>  		struct page *page;
> -		void *tmp;
> 
>  		entry = xa_load(&dmirror->pt, pfn);
>  		page = xa_untag_pointer(entry);
>  		if (!page)
>  			return -ENOENT;
> 
> -		tmp = kmap(page);
> -		memcpy(ptr, tmp, PAGE_SIZE);
> -		kunmap(page);
> +		memcpy_from_page(ptr, page, 0, PAGE_SIZE);
> 
>  		ptr += PAGE_SIZE;
>  		bounce->cpages++;
> @@ -437,16 +434,13 @@ static int dmirror_do_write(struct dmirror *dmirror,
> unsigned long start, for (pfn = start >> PAGE_SHIFT; pfn < (end >>
> PAGE_SHIFT); pfn++) { void *entry;
>  		struct page *page;
> -		void *tmp;
> 
>  		entry = xa_load(&dmirror->pt, pfn);
>  		page = xa_untag_pointer(entry);
>  		if (!page || xa_pointer_tag(entry) != DPT_XA_TAG_WRITE)
>  			return -ENOENT;
> 
> -		tmp = kmap(page);
> -		memcpy(tmp, ptr, PAGE_SIZE);
> -		kunmap(page);
> +		memcpy_to_page(page, 0, ptr, PAGE_SIZE);
> 
>  		ptr += PAGE_SIZE;
>  		bounce->cpages++;
> --
> 2.25.1

Re: [PATCH v2] lib: Replace kmap() with kmap_local_page()

[Sumitra Sharma]

On Sun, Jun 18, 2023 at 06:50:04AM +0200, Fabio M. De Francesco wrote:
> On sabato 10 giugno 2023 19:57:12 CEST Sumitra Sharma wrote:
> > kmap() has been deprecated in favor of the kmap_local_page()
> > due to high cost, restricted mapping space, the overhead of
> > a global lock for synchronization, and making the process
> > sleep in the absence of free slots.
> > 
> > kmap_local_page() is faster than kmap() and offers thread-local
> > and CPU-local mappings, take pagefaults in a local kmap region
> > and preserves preemption by saving the mappings of outgoing
> > tasks and restoring those of the incoming one during a context
> > switch.
> > 
> > The mappings are kept thread local in the functions
> > “dmirror_do_read” and “dmirror_do_write” in test_hmm.c
> > 
> > Therefore, replace kmap() with kmap_local_page() and use
> > mempcy_from/to_page() to avoid open coding kmap_local_page()
> > + memcpy() + kunmap_local().
> > 
> > Remove the unused variable “tmp”.
> > 
> > 
> > Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> LGTM, so...
> 
> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Thanks,
> 
> Fabio
> 
> P.S.: The answer to an old question from you is that "LGTM" stands for "[It] 
> Looks Good To Me". 
> 
> It's just a way to introduce the "Reviewed-by" tag itself. However, "LGTM" is 
> not required, whereas the tag is required for a valid review and only the tag 
> line will be added by maintainers in the log when your patch gets applied.  
> 
> While here... Please don't put unnecessary blank lines between the tags.They 
> are not required and instead may worsen readability (obviously, I'm not 
> requiring a new version for this).  
> 

Hi Fabio,

Thank you for your explanation and clarification regarding the meaning of "LGTM." 
I will take care of the blank lines.


Thanks & regards
Sumitra

> > 
> > Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> > ---
> > 
> > Changes in v2:
> > 	- Change commit subject and description.
> > 	- Remove unnecessary type casting (char *).
> > 
> > 
> >  lib/test_hmm.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> > index 67e6f83fe0f8..717dcb830127 100644
> > --- a/lib/test_hmm.c
> > +++ b/lib/test_hmm.c
> > @@ -368,16 +368,13 @@ static int dmirror_do_read(struct dmirror *dmirror,
> > unsigned long start, for (pfn = start >> PAGE_SHIFT; pfn < (end >>
> > PAGE_SHIFT); pfn++) { void *entry;
> >  		struct page *page;
> > -		void *tmp;
> > 
> >  		entry = xa_load(&dmirror->pt, pfn);
> >  		page = xa_untag_pointer(entry);
> >  		if (!page)
> >  			return -ENOENT;
> > 
> > -		tmp = kmap(page);
> > -		memcpy(ptr, tmp, PAGE_SIZE);
> > -		kunmap(page);
> > +		memcpy_from_page(ptr, page, 0, PAGE_SIZE);
> > 
> >  		ptr += PAGE_SIZE;
> >  		bounce->cpages++;
> > @@ -437,16 +434,13 @@ static int dmirror_do_write(struct dmirror *dmirror,
> > unsigned long start, for (pfn = start >> PAGE_SHIFT; pfn < (end >>
> > PAGE_SHIFT); pfn++) { void *entry;
> >  		struct page *page;
> > -		void *tmp;
> > 
> >  		entry = xa_load(&dmirror->pt, pfn);
> >  		page = xa_untag_pointer(entry);
> >  		if (!page || xa_pointer_tag(entry) != DPT_XA_TAG_WRITE)
> >  			return -ENOENT;
> > 
> > -		tmp = kmap(page);
> > -		memcpy(tmp, ptr, PAGE_SIZE);
> > -		kunmap(page);
> > +		memcpy_to_page(page, 0, ptr, PAGE_SIZE);
> > 
> >  		ptr += PAGE_SIZE;
> >  		bounce->cpages++;
> > --
> > 2.25.1
> 
> 
> 
>