Received-SPF: pass (google.com: domain of linux-kernel+bounces-156174-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240420-bpf_wq-v2-0-6c986a5a741f@kernel.org> <20240420-bpf_wq-v2-9-6c986a5a741f@kernel.org>
In-Reply-To: <20240420-bpf_wq-v2-9-6c986a5a741f@kernel.org>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 23 Apr 2024 19:51:17 -0700
Message-ID: <CAADnVQLO=QbXnDBKrDdofpkBg2jESC8rDwi_WiLNOahJQcEWSQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next v2 09/16] bpf: allow struct bpf_wq to be embedded
 in arraymaps and hashmaps
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, 
	Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, John Fastabend <john.fastabend@gmail.com>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>, 
	bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 20, 2024 at 2:09=E2=80=AFAM Benjamin Tissoires <bentiss@kernel.=
org> wrote:
>
>
> -static void htab_map_free_timers(struct bpf_map *map)
> +static void htab_map_free_timers_and_wq(struct bpf_map *map)
>  {
>         struct bpf_htab *htab =3D container_of(map, struct bpf_htab, map)=
;
>
> -       /* We only free timer on uref dropping to zero */
> -       if (!btf_record_has_field(htab->map.record, BPF_TIMER))
> -               return;
> -       if (!htab_is_prealloc(htab))
> -               htab_free_malloced_timers(htab);
> -       else
> -               htab_free_prealloced_timers(htab);
> +       /* We only free timer and workqueue on uref dropping to zero */
> +       if (btf_record_has_field(htab->map.record, BPF_TIMER)) {
> +               if (!htab_is_prealloc(htab))
> +                       htab_free_malloced_timers_or_wq(htab, true);
> +               else
> +                       htab_free_prealloced_timers(htab);
> +       }
> +       if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) {
> +               if (!htab_is_prealloc(htab))
> +                       htab_free_malloced_timers_or_wq(htab, false);
> +               else
> +                       htab_free_prealloced_wq(htab);
> +       }

I missed this earlier, but pls follow up to improve this part.
Double walk of all elements once for timer and 2nd for wq
is inefficient. Better to handle it in one go in prealloc and malloced case=
s.
Until we have delayed_wq it's quite possible that somebody will
have both timers and wq in map elements.