* [PATCH] lib/test_bpf: Replace kmap() with kmap_local_page()
@ 2023-06-12 10:33 Sumitra Sharma
2023-06-12 11:49 ` Fabio M. De Francesco
0 siblings, 1 reply; 3+ messages in thread
From: Sumitra Sharma @ 2023-06-12 10:33 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel
Cc: Ira Weiny, Fabio, Deepak R Varma
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 mapping is kept thread local in the function
“generate_test_data” in test_bpf.c
Therefore, replace kmap() with kmap_local_page() and use
memcpy_to_page() to avoid open coding kmap_local_page()
+ memcpy() + kunmap_local().
Remove the unused variable “ptr”.
Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---
lib/test_bpf.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index ade9ac672adb..3bb94727d83b 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -14381,25 +14381,17 @@ static void *generate_test_data(struct bpf_test *test, int sub)
* single fragment to the skb, filled with
* test->frag_data.
*/
- void *ptr;
-
page = alloc_page(GFP_KERNEL);
if (!page)
goto err_kfree_skb;
- ptr = kmap(page);
- if (!ptr)
- goto err_free_page;
- memcpy(ptr, test->frag_data, MAX_DATA);
- kunmap(page);
+ memcpy_to_page(page, 0, test->frag_data, MAX_DATA);
skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
}
return skb;
-err_free_page:
- __free_page(page);
err_kfree_skb:
kfree_skb(skb);
return NULL;
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] lib/test_bpf: Replace kmap() with kmap_local_page()
2023-06-12 10:33 [PATCH] lib/test_bpf: Replace kmap() with kmap_local_page() Sumitra Sharma
@ 2023-06-12 11:49 ` Fabio M. De Francesco
2023-06-13 7:30 ` Sumitra Sharma
0 siblings, 1 reply; 3+ messages in thread
From: Fabio M. De Francesco @ 2023-06-12 11:49 UTC (permalink / raw)
To: Sumitra Sharma
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel, Ira Weiny, Deepak R Varma
On lunedì 12 giugno 2023 12:33:41 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 mapping is kept thread local in the function
> “generate_test_data” in test_bpf.c
>
> Therefore, replace kmap() with kmap_local_page() and use
> memcpy_to_page() to avoid open coding kmap_local_page()
> + memcpy() + kunmap_local().
>
> Remove the unused variable “ptr”.
Sumitra,
Please Cc your mentors while in the internship.
It's not mandatory but it would expedite comments and reviews :-)
>
> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> ---
> lib/test_bpf.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index ade9ac672adb..3bb94727d83b 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -14381,25 +14381,17 @@ static void *generate_test_data(struct bpf_test
> *test, int sub) * single fragment to the skb, filled with
> * test->frag_data.
> */
> - void *ptr;
> -
> page = alloc_page(GFP_KERNEL);
>
> if (!page)
> goto err_kfree_skb;
>
> - ptr = kmap(page);
> - if (!ptr)
> - goto err_free_page;
> - memcpy(ptr, test->frag_data, MAX_DATA);
> - kunmap(page);
> + memcpy_to_page(page, 0, test->frag_data, MAX_DATA);
Why are you temporary mapping a page allocated with the GFP_KERNEL flag?
It cannot come from ZONE_HIGHMEM.
Fabio
> skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
> }
>
> return skb;
>
> -err_free_page:
> - __free_page(page);
> err_kfree_skb:
> kfree_skb(skb);
> return NULL;
> --
> 2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] lib/test_bpf: Replace kmap() with kmap_local_page()
2023-06-12 11:49 ` Fabio M. De Francesco
@ 2023-06-13 7:30 ` Sumitra Sharma
0 siblings, 0 replies; 3+ messages in thread
From: Sumitra Sharma @ 2023-06-13 7:30 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel, Ira Weiny, Deepak R Varma
On Mon, Jun 12, 2023 at 01:49:56PM +0200, Fabio M. De Francesco wrote:
> On lunedì 12 giugno 2023 12:33:41 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 mapping is kept thread local in the function
> > “generate_test_data” in test_bpf.c
> >
> > Therefore, replace kmap() with kmap_local_page() and use
> > memcpy_to_page() to avoid open coding kmap_local_page()
> > + memcpy() + kunmap_local().
> >
> > Remove the unused variable “ptr”.
>
> Sumitra,
>
> Please Cc your mentors while in the internship.
> It's not mandatory but it would expedite comments and reviews :-)
>
Hi Fabio,
I will take care of it.
> >
> > Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> > ---
> > lib/test_bpf.c | 10 +---------
> > 1 file changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> > index ade9ac672adb..3bb94727d83b 100644
> > --- a/lib/test_bpf.c
> > +++ b/lib/test_bpf.c
> > @@ -14381,25 +14381,17 @@ static void *generate_test_data(struct bpf_test
> > *test, int sub) * single fragment to the skb, filled with
> > * test->frag_data.
> > */
> > - void *ptr;
> > -
> > page = alloc_page(GFP_KERNEL);
> >
> > if (!page)
> > goto err_kfree_skb;
> >
> > - ptr = kmap(page);
> > - if (!ptr)
> > - goto err_free_page;
> > - memcpy(ptr, test->frag_data, MAX_DATA);
> > - kunmap(page);
> > + memcpy_to_page(page, 0, test->frag_data, MAX_DATA);
>
> Why are you temporary mapping a page allocated with the GFP_KERNEL flag?
> It cannot come from ZONE_HIGHMEM.
>
Fabio, I somewhere _wrongly_ read that "GFP_KERNEL may allocate pages from
highmem". However, with further readings and checking the GFP_KERNEL
definition in gfp_types.h
#define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS)
I have got that with the GFP_KERNEL flag, pages can never come
from ZONE_HIGHMEM.
Apologise.
Thanks & regards
Sumitra
> Fabio
>
> > skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
> > }
> >
> > return skb;
> >
> > -err_free_page:
> > - __free_page(page);
> > err_kfree_skb:
> > kfree_skb(skb);
> > return NULL;
> > --
> > 2.25.1
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-13 7:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-12 10:33 [PATCH] lib/test_bpf: Replace kmap() with kmap_local_page() Sumitra Sharma
2023-06-12 11:49 ` Fabio M. De Francesco
2023-06-13 7:30 ` Sumitra Sharma
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).