Re: [PATCH bpf-next 1/7] bpf: Create links for BPF struct_ops maps.

[Martin KaFai Lau <martin.lau@linux.dev>]

On 2/15/23 12:24 PM, Kui-Feng Lee wrote:
> 
> On 2/15/23 10:44, Stanislav Fomichev wrote:
>> On 02/15, Kui-Feng Lee wrote:
>>> Thank you for the feedback.
>>
>>
>>> On 2/14/23 18:39, Stanislav Fomichev wrote:
>>> > On 02/14, Kui-Feng Lee wrote:
> [..]
>>> >
>>> > > +��� if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS)
>>> > > +������� return -EINVAL;
>>> > > +
>>> > > +��� link = kzalloc(sizeof(*link), GFP_USER);
>>> > > +��� if (!link) {
>>> > > +������� err = -ENOMEM;
>>> > > +������� goto err_out;
>>> > > +��� }
>>> > > +��� bpf_link_init(link, BPF_LINK_TYPE_STRUCT_OPS,
>>> > > &bpf_struct_ops_map_lops, NULL);
>>> > > +��� link->map = map;
>>> > > +
>>> > > +��� err = bpf_link_prime(link, &link_primer);
>>> > > +��� if (err)
>>> > > +������� goto err_out;
>>> > > +
>>> > > +��� return bpf_link_settle(&link_primer);
>>> > > +
>>> > > +err_out:
>>> > > +��� bpf_map_put(map);
>>> > > +��� kfree(link);
>>> > > +��� return err;
>>> > > +}
>>> > > +
>>> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> > > index cda8d00f3762..54e172d8f5d1 100644
>>> > > --- a/kernel/bpf/syscall.c
>>> > > +++ b/kernel/bpf/syscall.c
>>> > > @@ -2738,7 +2738,9 @@ static void bpf_link_free(struct bpf_link *link)
>>> > > ����� if (link->prog) {
>>> > > ��������� /* detach BPF program, clean up used resources */
>>> > > ��������� link->ops->release(link);
>>> > > -������� bpf_prog_put(link->prog);
>>> > > +������� if (link->type != BPF_LINK_TYPE_STRUCT_OPS)
>>> > > +����������� bpf_prog_put(link->prog);
>>> > > +������� /* The struct_ops links clean up map by them-selves. */
>>> >
>>> > Why not more generic:
>>> >
>>> > if (link->prog)
>>> > ����bpf_prog_put(link->prog);
>>> >
>>> > ?
>>> The `prog` and `map` functions are now occupying the same space. I'm afraid
>>> this check won't work.
>>
>> Hmm, good point. In this case: why not have separate prog/map pointers
>> instead of a union? Are we 100% sure struct_ops is unique enough
>> and there won't ever be another map-based links?
>>
> Good question!
> 
> bpf_link is used to attach a single BPF program with a hook now. This patch 
> takes things one step further, allowing the attachment of struct_ops. We can 
> think of it as attaching a set of BPF programs to a pre-existing set of hooks. I 
> would say that bpf_link now represents the attachments of a set of BPF programs 
> to a pre-defined set of hooks. The size of a set is one for the case of 
> attaching single BPF program.
> 
> Is creating a new map-based link indicative of introducing an entirely distinct 
> type of map that reflects a set of BPF programs? If so, why doesn't struct_ops 
> work? If it happens, we may need to create a function to validate the 
> attach_type associated with any given map type.
> 

I also prefer separating 'prog' and 'map' in the link. It may be only struct_ops 
link that has no prog now but the future link type may not have prog also, so 
testing link->prog is less tie-up to a specific link type. Once separated, it 
makes sense to push the link's specific field to a new struct, so the following 
(from Stan) makes more sense:

struct bpt_struct_ops_link {
     struct bpf_link link;
     struct bpf_map *map;
};

In bpf_link_free(), there is no need to do an extra check for 'link->type != 
BPF_LINK_TYPE_STRUCT_OPS'. bpf_struct_ops_map_link_release() can be done 
together in bpf_struct_ops_map_link_dealloc().