Re: [PATCH bpf-next v3 1/8] bpf: Maintain the refcount of struct_ops maps directly.

[Kui-Feng Lee <sinquersw@xxxxxxxxx>]

On 3/6/23 15:16, Martin KaFai Lau wrote:
> On 3/2/23 5:21 PM, Kui-Feng Lee wrote:
> > The refcount of the kvalue for struct_ops was quite intricate to keep
> > track of. By no longer utilizing it and replacing it with the refcount
> > from the struct_ops map, this process became more transparent and
> > uncomplicated.
>
> The patch's subject is not very clear. may be 'Retire the struct_ops map 
> kvalue->refcnt' better reflect what the patch is doing?
>
> The commit message also needs details on the major change and the reason 
> for the change. eg. Why freeing the struct_ops map needs to go through 
> the rcu grace period and it is the reason on the rcu related changes in 
> this patch.
> Why retiring kvalue->refcnt is needed for (or can simplify?) the later 
> patches?

Sure!

> > @@ -261,13 +264,13 @@ int bpf_struct_ops_map_sys_lookup_elem(struct 
> > bpf_map *map, void *key,
> >           return 0;
> >       }
> > -    /* No lock is needed.  state and refcnt do not need
> > -     * to be updated together under atomic context.
> > -     */
>
> This comment is still valid in this patch?
>
> >       uvalue = value;
> >       memcpy(uvalue, st_map->uvalue, map->value_size);
> >       uvalue->state = state;
> > -    refcount_set(&uvalue->refcnt, refcount_read(&kvalue->refcnt));
> > +
> > +    refcnt = atomic64_read(&map->refcnt) - atomic64_read(&map->usercnt);
> > +    refcount_set(&uvalue->refcnt,
> > +             refcnt > 0 ? refcnt : 0);
>
> nit. max_t().
>
> It also needs comment on why it will work or at least good enough.

Got it.

> >       return 0;
> >   }
> > @@ -491,7 +494,6 @@ static int bpf_struct_ops_map_update_elem(struct 
> > bpf_map *map, void *key,
> >           *(unsigned long *)(udata + moff) = prog->aux->id;
> >       }
> > -    refcount_set(&kvalue->refcnt, 1);
> >       bpf_map_inc(map);
> >       set_memory_rox((long)st_map->image, 1);
> > @@ -536,8 +538,7 @@ static int bpf_struct_ops_map_delete_elem(struct 
> > bpf_map *map, void *key)
> >       switch (prev_state) {
> >       case BPF_STRUCT_OPS_STATE_INUSE:
> >           st_map->st_ops->unreg(&st_map->kvalue.data);
> > -        if (refcount_dec_and_test(&st_map->kvalue.refcnt))
> > -            bpf_map_put(map);
> > +        bpf_map_put(map);
> >           return 0;
> >       case BPF_STRUCT_OPS_STATE_TOBEFREE:
> >           return -EINPROGRESS;
> > @@ -582,6 +583,38 @@ static void bpf_struct_ops_map_free(struct 
> > bpf_map *map)
> >       bpf_map_area_free(st_map);
> >   }
> > +static void bpf_struct_ops_map_free_wq(struct rcu_head *head)
> > +{
> > +    struct bpf_struct_ops_map *st_map;
> > +
> > +    st_map = container_of(head, struct bpf_struct_ops_map, rcu);
> > +
> > +    /* bpf_map_free_deferred should not be called in a RCU callback. */
> > +    INIT_WORK(&st_map->map.work, bpf_map_free_deferred);
> > +    queue_work(system_unbound_wq, &st_map->map.work);
> > +}
> > +
> > +static void bpf_struct_ops_map_free_rcu(struct bpf_map *map)
> > +{
> > +    struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map 
> > *)map;
> > +
> > +    /* Wait for a grace period of RCU. Then, post the map_free
> > +     * work to the system_unbound_wq workqueue to free resources.
> > +     *
> > +     * The struct_ops's function may switch to another struct_ops.
> > +     *
> > +     * For example, bpf_tcp_cc_x->init() may switch to
> > +     * another tcp_cc_y by calling
> > +     * setsockopt(TCP_CONGESTION, "tcp_cc_y").
> > +     * During the switch,  bpf_struct_ops_put(tcp_cc_x) is called
> > +     * and its refcount may reach 0 which then free its
> > +     * trampoline image while tcp_cc_x is still running.
> > +     *
> > +     * Thus, a rcu grace period is needed here.
> > +     */
> > +    call_rcu(&st_map->rcu, bpf_struct_ops_map_free_wq);
> > +}
> > +
> >   static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
> >   {
> >       if (attr->key_size != sizeof(unsigned int) || attr->max_entries 
> > != 1 ||
> > @@ -646,6 +679,7 @@ const struct bpf_map_ops bpf_struct_ops_map_ops = {
> >       .map_alloc_check = bpf_struct_ops_map_alloc_check,
> >       .map_alloc = bpf_struct_ops_map_alloc,
> >       .map_free = bpf_struct_ops_map_free,
> > +    .map_free_rcu = bpf_struct_ops_map_free_rcu,
>
> just came to my mind. Instead of having a rcu callback, 
> synchronize_rcu() can be called in bpf_struct_ops_map_free(). Then the 
> '.map_free_rcu' addition and its related change is not needed.

synchronize_rcu() probably blocks other subsystem, right?