Message-ID: <20210419214517.bqur27tytx4onfnn@apollo>
Date:   Tue, 20 Apr 2021 03:15:17 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     bpf@...r.kernel.org,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 3/4] libbpf: add low level TC-BPF API

On Tue, Apr 20, 2021 at 02:30:44AM IST, Daniel Borkmann wrote:
> On 4/19/21 2:18 PM, Kumar Kartikeya Dwivedi wrote:
> > This adds functions that wrap the netlink API used for adding,
> > manipulating, and removing traffic control filters. These functions
> > operate directly on the loaded prog's fd, and return a handle to the
> > filter using an out parameter named id.
> >
> > The basic featureset is covered to allow for attaching, manipulation of
> > properties, and removal of filters. Some additional features like
> > TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can
> > added on top later by extending the bpf_tc_cls_opts struct.
> >
> > Support for binding actions directly to a classifier by passing them in
> > during filter creation has also been omitted for now. These actions have
> > an auto clean up property because their lifetime is bound to the filter
> > they are attached to. This can be added later, but was omitted for now
> > as direct action mode is a better alternative to it, which is enabled by
> > default.
> >
> > An API summary:
> >
> > bpf_tc_act_{attach, change, replace} may be used to attach, change, and
>
> typo on bpf_tc_act_{...} ?
>

Oops, yes. Should be bpf_tc_cls_...

> > replace SCHED_CLS bpf classifier. The protocol field can be set as 0, in
> > which case it is subsitituted as ETH_P_ALL by default.
>
> Do you have an actual user that needs anything other than ETH_P_ALL? Why is it
> even needed? Why not stick to just ETH_P_ALL?
>

Mostly because it was little to no effort to expose this. Though if you feel
strongly about it I can drop the protocol option, and just bake in ETH_P_ALL. It
can always be added later ofcourse, if the need arises in the future.

> > The behavior of the three functions is as follows:
> >
> > attach = create filter if it does not exist, fail otherwise
> > change = change properties of the classifier of existing filter
> > replace = create filter, and replace any existing filter
>
> This touches on tc oddities quite a bit. Why do we need to expose them? Can't we
> simplify/abstract this e.g. i) create or update instance, ii) delete instance,
> iii) get instance ? What concrete use case do you have that you need those three
> above?
>

'change' is relevant for modifying classifier specific options, and given it's
a lot less useful now as per the current state of this patch, I am fine with
removing it. This is also where the distinction becomes visible to the user, so
removing it should hide the filter/classifier separation.

> > bpf_tc_cls_detach may be used to detach existing SCHED_CLS
> > filter. The bpf_tc_cls_attach_id object filled in during attach,
> > change, or replace must be passed in to the detach functions for them to
> > remove the filter and its attached classififer correctly.
> >
> > bpf_tc_cls_get_info is a helper that can be used to obtain attributes
> > for the filter and classififer. The opts structure may be used to
> > choose the granularity of search, such that info for a specific filter
> > corresponding to the same loaded bpf program can be obtained. By
> > default, the first match is returned to the user.
> >
> > Examples:
> >
> > 	struct bpf_tc_cls_attach_id id = {};
> > 	struct bpf_object *obj;
> > 	struct bpf_program *p;
> > 	int fd, r;
> >
> > 	obj = bpf_object_open("foo.o");
> > 	if (IS_ERR_OR_NULL(obj))
> > 		return PTR_ERR(obj);
> >
> > 	p = bpf_object__find_program_by_title(obj, "classifier");
> > 	if (IS_ERR_OR_NULL(p))
> > 		return PTR_ERR(p);
> >
> > 	if (bpf_object__load(obj) < 0)
> > 		return -1;
> >
> > 	fd = bpf_program__fd(p);
> >
> > 	r = bpf_tc_cls_attach(fd, if_nametoindex("lo"),
> > 			      BPF_TC_CLSACT_INGRESS,
> > 			      NULL, &id);
> > 	if (r < 0)
> > 		return r;
> >
> > ... which is roughly equivalent to (after clsact qdisc setup):
> >    # tc filter add dev lo ingress bpf obj foo.o sec classifier da
> >
> > ... as direct action mode is always enabled.
> >
> > If a user wishes to modify existing options on an attached classifier,
> > bpf_tc_cls_change API may be used.
> >
> > Only parameters class_id can be modified, the rest are filled in to
> > identify the correct filter. protocol can be left out if it was not
> > chosen explicitly (defaulting to ETH_P_ALL).
> >
> > Example:
> >
> > 	/* Optional parameters necessary to select the right filter */
> > 	DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts,
> > 			    .handle = id.handle,
> > 			    .priority = id.priority,
> > 			    .chain_index = id.chain_index)
>
> Why do we need chain_index as part of the basic API?
>

It would be relevant when TC_ACT_GOTO_CHAIN is used. Other than that, I guess
it's not very useful.

> > 	opts.class_id = TC_H_MAKE(1UL << 16, 12);
> > 	r = bpf_tc_cls_change(fd, if_nametoindex("lo"),
> > 			      BPF_TC_CLSACT_INGRESS,
> > 			      &opts, &id);
>
> Also, I'm not sure whether the prefix should even be named  bpf_tc_cls_*() tbh,
> yes, despite being "low level" api. I think in the context of bpf we should stop
> regarding this as 'classifier' and 'action' objects since it's really just a
> single entity and not separate ones. It's weird enough to explain this concept
> to new users and if a libbpf based api could cleanly abstract it, I would be all
> for it. I don't think we need to map 1:1 the old tc legacy even in the low level
> api, tbh, as it feels backwards. I think the 'handle' & 'priority' bits are okay,
> but I would remove the others.
>

Ok, would dropping _cls from the name be better?
bpf_tc_attach
bpf_tc_replace
bpf_tc_get_info
bpf_tc_detach

As for options, I'll drop protocol, if you feel strongly about chain_index I can
drop that one too.

> > 	if (r < 0)
> > 		return r;
> >
> > 	struct bpf_tc_cls_info info = {};
> > 	r = bpf_tc_cls_get_info(fd, if_nametoindex("lo"),
> > 			        BPF_TC_CLSACT_INGRESS,
> > 				&opts, &info);
> > 	if (r < 0)
> > 		return r;
> >
> > 	assert(info.class_id == TC_H_MAKE(1UL << 16, 12));
> >
> > This would be roughly equivalent to doing:
> >    # tc filter change dev lo egress prio <p> handle <h> bpf obj foo.o sec \
> >      classifier classid 1:12
>
> Why even bother to support !da mode, what are you trying to solve with it? I
> don't think official libbpf api should support something that doesn't scale.
>

da is default now, this is yet another typo/oversight...

--
Kartikeya

Powered by blists - more mailing lists