Hi,
On 7/6/2023 11:34 AM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> alloc_bulk() can reuse elements from free_by_rcu_ttrace.
> Let it reuse from waiting_for_gp_ttrace as well to avoid unnecessary kmalloc().
>
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
> kernel/bpf/memalloc.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
> index 9986c6b7df4d..e5a87f6cf2cc 100644
> --- a/kernel/bpf/memalloc.c
> +++ b/kernel/bpf/memalloc.c
> @@ -212,6 +212,15 @@ static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node)
> if (i >= cnt)
> return;
>
> + for (; i < cnt; i++) {
> + obj = llist_del_first(&c->waiting_for_gp_ttrace);
> + if (!obj)
> + break;
> + add_obj_to_free_list(c, obj);
> + }
> + if (i >= cnt)
> + return;
I still think using llist_del_first() here is not safe as reported in
[1]. Not sure whether or not invoking enque_to_free() firstly for
free_llist_extra will close the race completely. Will check later.
[1]:
https://lore.kernel.org/rcu/957dd5cd-0855-1197-7045-4cb1590bd753@xxxxxxxxxxxxxxx/
> +
> memcg = get_memcg(c);
> old_memcg = set_active_memcg(memcg);
> for (; i < cnt; i++) {
> @@ -295,12 +304,7 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>
> WARN_ON_ONCE(!llist_empty(&c->waiting_for_gp_ttrace));
> llist_for_each_safe(llnode, t, llist_del_all(&c->free_by_rcu_ttrace))
> - /* There is no concurrent __llist_add(waiting_for_gp_ttrace) access.
> - * It doesn't race with llist_del_all either.
> - * But there could be two concurrent llist_del_all(waiting_for_gp_ttrace):
> - * from __free_rcu() and from drain_mem_cache().
> - */
> - __llist_add(llnode, &c->waiting_for_gp_ttrace);
> + llist_add(llnode, &c->waiting_for_gp_ttrace);
>
> if (unlikely(READ_ONCE(c->draining))) {
> __free_rcu(&c->rcu_ttrace);
