linux-kernel - Re: [PATCH v2] lib: Replace kmap() with kmap_local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8275009.NyiUUSuA9g@suse>
Date:   Sun, 18 Jun 2023 06:50:04 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Jérôme Glisse <jglisse@...hat.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Sumitra Sharma <sumitraartsy@...il.com>
Cc:     Ira Weiny <ira.weiny@...el.com>, Deepak R Varma <drv@...lo.com>
Subject: Re: [PATCH v2] lib: Replace kmap() with kmap_local_page()

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@...il.com>

LGTM, so...

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@...il.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@...il.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