Thanks everyone for the reviews and suggestions! On Wed, Dec 18, 2019 at 10:00:44AM -0800, Alexei Starovoitov wrote: > On Wed, Dec 18, 2019 at 03:23:04PM +0100, Paul Chaignon wrote: > > Currently, userspace programs have to update the values of all CPUs at > > once when updating per-cpu maps. This limitation prevents the update of > > a single CPU's value without the risk of missing concurrent updates on > > other CPU's values. > > > > This patch allows userspace to update the value of a specific CPU in > > per-cpu maps. The CPU whose value should be updated is encoded in the > > 32 upper-bits of the flags argument, as follows. The new BPF_CPU flag > > can be combined with existing flags. > > In general makes sense. Could you elaborate more on concrete issue? Sure. I have a BPF program that matches incoming packets against packet-filtering rules, with a NIC that steers some flows (correspond to different tenants) to specific CPUs. Rules are stored in a per-cpu hashmap with a counter (counting matching packets) and an off/on switch. To enable a rule on a new CPU, I lookup the value for that rule, switch the on/off variable in the value corresponding to the given CPU and update the map. However, the counters corresponding to other CPUs for that same rule might have been updated between the lookup and the update. Other BPF users have requested the same feature before on the bcc repository [1]. They probably have different (tracing-related?) use cases. 1 - https://github.com/iovisor/bcc/issues/1886 > > > bpf_map_update_elem(..., cpuid << 32 | BPF_CPU) > > > > Signed-off-by: Paul Chaignon <paul.chaignon@xxxxxxxxxx> > > --- > > include/uapi/linux/bpf.h | 4 +++ > > kernel/bpf/arraymap.c | 19 ++++++++----- > > kernel/bpf/hashtab.c | 49 ++++++++++++++++++++-------------- > > kernel/bpf/local_storage.c | 16 +++++++---- > > kernel/bpf/syscall.c | 17 +++++++++--- > > tools/include/uapi/linux/bpf.h | 4 +++ > > 6 files changed, 74 insertions(+), 35 deletions(-) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index dbbcf0b02970..2efb17d2c77a 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -316,6 +316,10 @@ enum bpf_attach_type { > > #define BPF_NOEXIST 1 /* create new element if it didn't exist */ > > #define BPF_EXIST 2 /* update existing element */ > > #define BPF_F_LOCK 4 /* spin_lock-ed map_lookup/map_update */ > > +#define BPF_CPU 8 /* single-cpu update for per-cpu maps */ > > BPF_F_CPU would be more consistent with the rest of flags. > > Can BPF_F_CURRENT_CPU be supported as well? You mean to update the value corresponding to the CPU on which the userspace program is running? BPF_F_CURRENT_CPU is a mask on the lower 32 bits so it would clash with existing flags, but there's probably another way to implement the same. Not sure I see the use case though; userspace programs can easily update the value for the current CPU with BPF_F_CPU. > > And for consistency support this flag in map_lookup_elem too? Sure, I'll add it to the v2. > > > + > > +/* CPU mask for single-cpu updates */ > > +#define BPF_CPU_MASK 0xFFFFFFFF00000000ULL > > what is the reason to expose this in uapi? No reason; that's a mistake. > > > /* flags for BPF_MAP_CREATE command */ > > #define BPF_F_NO_PREALLOC (1U << 0) > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > > index f0d19bbb9211..a96e94696819 100644 > > --- a/kernel/bpf/arraymap.c > > +++ b/kernel/bpf/arraymap.c > > @@ -302,7 +302,8 @@ static int array_map_update_elem(struct bpf_map *map, void *key, void *value, > > u32 index = *(u32 *)key; > > char *val; > > > > - if (unlikely((map_flags & ~BPF_F_LOCK) > BPF_EXIST)) > > + if (unlikely((map_flags & ~BPF_CPU_MASK & ~BPF_F_LOCK & > > + ~BPF_CPU) > BPF_EXIST)) > > that reads odd. > More traditional would be ~ (A | B | C) >
