[bpf-next,v5,7/7] bpf: Use try_alloc_pages() to allocate pages for bpf needs.

Commit Message

Alexei Starovoitov Jan. 15, 2025, 2:17 a.m. UTC

From: Alexei Starovoitov <[e-mail]>

Use try_alloc_pages() and free_pages_nolock()

Signed-off-by: Alexei Starovoitov <[e-mail]>
---
 kernel/bpf/syscall.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Vlastimil Babka Jan. 15, 2025, 6:02 p.m. UTC | #1

On 1/15/25 03:17, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> Use try_alloc_pages() and free_pages_nolock()
> 
> Signed-off-by: Alexei Starovoitov <[e-mail]>
> ---
>  kernel/bpf/syscall.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 0daf098e3207..8bcf48e31a5a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -582,14 +582,14 @@ int bpf_map_alloc_pages(const struct bpf_map *map, gfp_t gfp, int nid,

This makes the gfp parameter unused? And the callers are passing GFP_KERNEL
anyway? Isn't try_alloc_pages() rather useful for some context that did not
even try to allocate until now, but now it could?

Also unless my concerns about page_owner were wrong, this is where they
could manifest.

>  	old_memcg = set_active_memcg(memcg);
>  #endif
>  	for (i = 0; i < nr_pages; i++) {
> -		pg = alloc_pages_node(nid, gfp | __GFP_ACCOUNT, 0);
> +		pg = try_alloc_pages(nid, 0);
>  
>  		if (pg) {
>  			pages[i] = pg;
>  			continue;
>  		}
>  		for (j = 0; j < i; j++)
> -			__free_page(pages[j]);
> +			free_pages_nolock(pages[j], 0);
>  		ret = -ENOMEM;
>  		break;
>  	}

Alexei Starovoitov Jan. 16, 2025, 2:25 a.m. UTC | #2

On Wed, Jan 15, 2025 at 10:03 AM Vlastimil Babka <[e-mail]> wrote:
>
> On 1/15/25 03:17, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <[e-mail]>
> >
> > Use try_alloc_pages() and free_pages_nolock()
> >
> > Signed-off-by: Alexei Starovoitov <[e-mail]>
> > ---
> >  kernel/bpf/syscall.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 0daf098e3207..8bcf48e31a5a 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -582,14 +582,14 @@ int bpf_map_alloc_pages(const struct bpf_map *map, gfp_t gfp, int nid,
>
> This makes the gfp parameter unused? And the callers are passing GFP_KERNEL
> anyway? Isn't try_alloc_pages() rather useful for some context that did not
> even try to allocate until now, but now it could?

Correct. gfp arg is unused and currently it's only called
from sleepable bpf kfunc with GFP_KERNEL.
I have more patches on top that change all that:
remove gfp argument, etc.
Just didn't send them as part of this set, since it's all bpf internal
stuff.

Patch

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0daf098e3207..8bcf48e31a5a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -582,14 +582,14 @@  int bpf_map_alloc_pages(const struct bpf_map *map, gfp_t gfp, int nid,
 	old_memcg = set_active_memcg(memcg);
 #endif
 	for (i = 0; i < nr_pages; i++) {
-		pg = alloc_pages_node(nid, gfp | __GFP_ACCOUNT, 0);
+		pg = try_alloc_pages(nid, 0);
 
 		if (pg) {
 			pages[i] = pg;
 			continue;
 		}
 		for (j = 0; j < i; j++)
-			__free_page(pages[j]);
+			free_pages_nolock(pages[j], 0);
 		ret = -ENOMEM;
 		break;
 	}