On Tue, Feb 11, 2025 at 2:59 AM Ritesh Oedayrajsingh Varma
<ritesh@xxxxxxxxxxxxxxx> wrote:
>
> On Sat, Feb 8, 2025 at 5:15 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Sat, Feb 8, 2025 at 2:39 AM Ritesh Oedayrajsingh Varma
> > <ritesh@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Sat, Feb 8, 2025 at 4:58 AM Alexei Starovoitov
> > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > >
> > > > On Wed, Feb 5, 2025 at 4:58 AM Ritesh Oedayrajsingh Varma
> > > > <ritesh@xxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > Given this, while it's not possible to remove the wait entirely
> > > > > without breaking user space, I was wondering if it would be
> > > > > possible/acceptable to add a way to opt-out of this behavior for
> > > > > programs like ours that don't care about this. One way to do so could
> > > > > be to add an additional flag to the BPF_MAP_CREATE flags, perhaps
> > > > > something like BPF_F_INNER_MAP_NO_SYNC.
> > > >
> > > > Sounds reasonable.
> > > > The flag name is a bit cryptic, BPF_F_UPDATE_INNER_MAP_NO_WAIT
> > > > is a bit more explicit, but I'm not sure whether it's better.
> > >
> > > I agree the name is a bit cryptic. A related question is whether the
> > > optimization to skip the RCU wait should only apply to the update, or
> > > also to delete. I think it would make sense for it to apply to all
> > > operations. What do you think?
> > >
> > > I also realized the flag should technically apply to the *outer* map,
> > > since that's the map that's actually being modified and synced on, not
> > > the inner map. So I don't think "inner" should be part of the name in
> > > retrospect.
> >
> > BPF_F_UPDATE_INNER_MAP_NO_WAIT suppose to mean update_OF_inner_map.
> >
> > > Perhaps BPF_F_MAP_OF_MAPS_NO_WAIT or
> > > BPF_F_MAP_IN_MAP_NO_WAIT? I'm slightly leaning towards the latter
> > > because the map of maps support code is also located in map_in_map.c,
> > > so that matches nicely. They're both a bit long though. Either way,
> > > the definition of the outer map when using this flag would become
> > > something like:
> > >
> > > struct {
> > > __uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
> > > __uint(max_entries, 4096);
> > > __type(key, u64);
> > > __type(value, u32);
> > > __uint(map_flags, BPF_F_MAP_IN_MAP_NO_WAIT);
> > > } mapInMap SEC(".maps");
> >
> > Actually we probably should make it similar to BPF_NOEXIST/ANY flag,
> > so it can be passed to update/delete command instead of
> > being a global property of the map.
> >
> > BPF_F_MAP_IN_MAP_NO_WAIT name sounds fine.
>
> Thanks. Making it part of the update/delete flags makes sense. There
> is already something similar in BPF_F_LOCK. Looking at the code, that
> does mean it would probably make the most sense to perform the check
> for such a flag outside of maybe_wait_bpf_programs() and make the
> calls to maybe_wait_bpf_programs() conditional on the flag in
> map_update_elem() / map_delete_elem() / bpf_map_do_batch().
correct.
> >
> > But before we add the uapi flag please benchmark
> > synchronize_rcu_expedited() as Hou suggested.
>
> I'll need to find some time for this, but will do.
pls do.
