From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-187.mta1.migadu.com (out-187.mta1.migadu.com [95.215.58.187]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 91FFB220F5C for ; Wed, 17 Sep 2025 15:20:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.187 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1758122447; cv=none; b=n2TAiNMKLgwisvTUf6KqDHU1uFzkpHS84a3KKfpBkZAU3HFeIpMTAzl5+UWxxmtqujCqK+3leKR0BMoa55DvdlVwF27yag1/IABz8O+NIe8A36HbqmwZDt6nF5fbpv9VhgehPa0eGrlvkzmz59Nk3bhq1BsPbx2oteayj9sl9tA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1758122447; c=relaxed/simple; bh=gMdSqAEEEsjkYj2Ceed5SV+H9JhS/KFMTz4H11G8VKQ=; h=Mime-Version:Content-Type:Date:Message-Id:Cc:Subject:From:To: References:In-Reply-To; b=Yv0USEoYTsZmLhl1N+AUPcectCNjEBR5cfOhIgmqA004/CTlVsL7KCSWDJPvLJh9O8BtHhFBz81RcuJoANaTdJ+eBFypgXWszaePUfBTLl8aZv4HyKkQAPyHOUdHZ48sY2lC3BoOa0ROCveFavedAINPXZSwTRraZ0jvdAaZJfM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=ct1JHilP; arc=none smtp.client-ip=95.215.58.187 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="ct1JHilP" Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1758122443; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=DY8gkbqS/73+HuB83mzIlstmWTi0FZ8tJESlDO3SN/A=; b=ct1JHilPiaT258ZKSwK7lw3zpuzzdiceGsNE0PPtQ+cXG8o5BM9EU9uUAicoEX1ckeC9+R HCWIKVKhd42uDUrnhtOE0/WtU/kqMUKRV20QKl7CB0TYusdaJCahxfn/JVPdtcBsgICBdb ya15DA+QASwKPlK5cRYmO0uac9GFvcM= Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 17 Sep 2025 23:20:35 +0800 Message-Id: Cc: , , , , , , , , , , Subject: Re: [PATCH bpf-next v7 4/7] bpf: Add BPF_F_CPU and BPF_F_ALL_CPUS flags support for percpu_hash and lru_percpu_hash maps X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: "Leon Hwang" To: "Andrii Nakryiko" References: <20250910162733.82534-1-leon.hwang@linux.dev> <20250910162733.82534-5-leon.hwang@linux.dev> In-Reply-To: X-Migadu-Flow: FLOW_OUT On Wed Sep 17, 2025 at 7:44 AM +08, Andrii Nakryiko wrote: > On Wed, Sep 10, 2025 at 9:28=E2=80=AFAM Leon Hwang = wrote: >> >> Introduce BPF_F_ALL_CPUS flag support for percpu_hash and lru_percpu_has= h >> maps to allow updating values for all CPUs with a single value for both >> update_elem and update_batch APIs. >> >> Introduce BPF_F_CPU flag support for percpu_hash and lru_percpu_hash >> maps to allow: >> >> * update value for specified CPU for both update_elem and update_batch >> APIs. >> * lookup value for specified CPU for both lookup_elem and lookup_batch >> APIs. >> >> The BPF_F_CPU flag is passed via: >> >> * map_flags along with embedded cpu info. >> * elem_flags along with embedded cpu info. >> >> Signed-off-by: Leon Hwang >> --- >> include/linux/bpf.h | 4 ++- >> kernel/bpf/hashtab.c | 77 +++++++++++++++++++++++++++++++------------- >> kernel/bpf/syscall.c | 2 +- >> 3 files changed, 58 insertions(+), 25 deletions(-) >> > > [...] > >> @@ -1147,7 +1158,7 @@ static long htab_map_update_elem(struct bpf_map *m= ap, void *key, void *value, >> } >> >> l_new =3D alloc_htab_elem(htab, key, value, key_size, hash, fals= e, false, >> - l_old); >> + l_old, map_flags); >> if (IS_ERR(l_new)) { >> /* all pre-allocated elements are in use or memory exhau= sted */ >> ret =3D PTR_ERR(l_new); >> @@ -1263,7 +1274,7 @@ static long htab_map_update_elem_in_place(struct b= pf_map *map, void *key, >> u32 key_size, hash; >> int ret; >> >> - if (unlikely(map_flags > BPF_EXIST)) >> + if (unlikely(!onallcpus && map_flags > BPF_EXIST)) > > BPF_F_LOCK shouldn't be let through > Ack. >> /* unknown flags */ >> return -EINVAL; >> > > [...] > >> @@ -1698,9 +1709,16 @@ __htab_map_lookup_and_delete_batch(struct bpf_map= *map, >> int ret =3D 0; >> >> elem_map_flags =3D attr->batch.elem_flags; >> - if ((elem_map_flags & ~BPF_F_LOCK) || >> - ((elem_map_flags & BPF_F_LOCK) && !btf_record_has_field(map-= >record, BPF_SPIN_LOCK))) >> - return -EINVAL; >> + if (!do_delete && is_percpu) { >> + ret =3D bpf_map_check_op_flags(map, elem_map_flags, BPF_= F_LOCK | BPF_F_CPU); >> + if (ret) >> + return ret; >> + } else { >> + if ((elem_map_flags & ~BPF_F_LOCK) || >> + ((elem_map_flags & BPF_F_LOCK) && >> + !btf_record_has_field(map->record, BPF_SPIN_LOCK))) >> + return -EINVAL; >> + } > > partially open-coded bpf_map_check_op_flags() if `do_delete || > !is_percpu`, right? Have you considered > > u32 allowed_flags =3D 0; > > ... > > allowed_flags =3D BPF_F_LOCK | BPF_F_CPU; > if (do_delete || !is_percpu) > allowed_flags ~=3D BPF_F_CPU; > err =3D bpf_map_check_op_flags(map, elem_map_flags, allowed_flags); > > > This reads way more natural (in my head...), and no open-coding the > helper you just so painstakingly extracted and extended to check all > these conditions. > My intention was to call bpf_map_check_op_flags() only for lookup_batch on *percpu* hash maps, while excluding lookup_batch on non-percpu hash maps and the lookup_and_delete_batch API. I don=E2=80=99t think we should be checking op flags for non-percpu hash ma= ps or for lookup_and_delete_batch cases. >> >> map_flags =3D attr->batch.flags; >> if (map_flags) >> @@ -1724,7 +1742,7 @@ __htab_map_lookup_and_delete_batch(struct bpf_map = *map, >> value_size =3D htab->map.value_size; >> size =3D round_up(value_size, 8); >> if (is_percpu) >> - value_size =3D size * num_possible_cpus(); >> + value_size =3D (elem_map_flags & BPF_F_CPU) ? size : siz= e * num_possible_cpus(); > > if (is_percpu && !(elem_map_flags & BPF_F_CPU)) > value_size =3D size * num_possible_cpus(); > > ? > Right, good catch. Thanks, Leon