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.
