On Fri, 9 Sept 2022 at 16:24, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Sep 9, 2022 at 4:05 AM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> >
> > On Fri, 9 Sept 2022 at 10:13, Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
> > >
> > > On 9/4/22 4:41 PM, Kumar Kartikeya Dwivedi wrote:
> > > > Global variables reside in maps accessible using direct_value_addr
> > > > callbacks, so giving each load instruction's rewrite a unique reg->id
> > > > disallows us from holding locks which are global.
> > > >
> > > > This is not great, so refactor the active_spin_lock into two separate
> > > > fields, active_spin_lock_ptr and active_spin_lock_id, which is generic
> > > > enough to allow it for global variables, map lookups, and local kptr
> > > > registers at the same time.
> > > >
> > > > Held vs non-held is indicated by active_spin_lock_ptr, which stores the
> > > > reg->map_ptr or reg->btf pointer of the register used for locking spin
> > > > lock. But the active_spin_lock_id also needs to be compared to ensure
> > > > whether bpf_spin_unlock is for the same register.
> > > >
> > > > Next, pseudo load instructions are not given a unique reg->id, as they
> > > > are doing lookup for the same map value (max_entries is never greater
> > > > than 1).
> > > >
> > >
> > > For libbpf-style "internal maps" - like .bss.private further in this series -
> > > all the SEC(".bss.private") vars are globbed together into one map_value. e.g.
> > >
> > > struct bpf_spin_lock lock1 SEC(".bss.private");
> > > struct bpf_spin_lock lock2 SEC(".bss.private");
> > > ...
> > > spin_lock(&lock1);
> > > ...
> > > spin_lock(&lock2);
> > >
> > > will result in same map but different offsets for the direct read (and different
> > > aux->map_off set in resolve_pseudo_ldimm64 for use in check_ld_imm). Seems like
> > > this patch would assign both same (active_spin_lock_ptr, active_spin_lock_id).
> > >
> >
> > That won't be a problem. Two spin locks in a map value or datasec are
> > already rejected on BPF_MAP_CREATE,
> > so there is no bug. See idx >= info_cnt check in
> > btf_find_struct_field, btf_find_datasec_var.
> >
> > I can include offset as the third part of the tuple. The problem then
> > is figuring out which lock protects which bpf_list_head. We need
> > another __guarded_by annotation and force users to use that to
> > eliminate the ambiguity. So for now I just put it in the commit log
> > and left it for the future.
>
> Let's not go that far yet.
> Extra annotations are just as confusing and non-obvious as
> putting locks in different sections.
> Let's keep one lock per map value limitation for now.
> libbpf side needs to allow many non-mappable sections though.
> Single bss.private name is too limiting.
In that case,
Dave, since the libbpf patch is yours, would you be fine with
reworking it to support multiple private maps?
Maybe it can just ignore the .XXX part in .bss.private.XXX?
Also I think Andrii mentioned once that he wants to eventually merge
data and bss, so it might be a good idea to call it .data.private from
the start?
