Re: [Patch net-next] net_sched: add reverse binding for tc class

[Daniel Borkmann <daniel@iogearbox.net>]

On 08/31/2017 12:22 AM, Daniel Borkmann wrote:
> On 08/31/2017 12:01 AM, Cong Wang wrote:
>> On Wed, Aug 30, 2017 at 2:48 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 08/30/2017 11:30 PM, Cong Wang wrote:
>>> [...]
>>>>
>>>> Note, we still can NOT totally get rid of those class lookup in
>>>> ->enqueue() because cgroup and flow filters have no way to determine
>>>> the classid at setup time, they still have to go through dynamic lookup.
>>>
>>> [...]
>>>>
>>>> ---
>>>>    include/net/sch_generic.h |  1 +
>>>>    net/sched/cls_basic.c     |  9 +++++++
>>>>    net/sched/cls_bpf.c       |  9 +++++++
>>>
>>> Same is for cls_bpf as well, so bind_class wouldn't work there
>>> either as we could return dynamic classids. bind_class cannot
>>> be added here, too.
>>
>> I think you are probably right, but the following code is
>> misleading there:
>>
>>          if (tb[TCA_BPF_CLASSID]) {
>>                  prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
>>                  tcf_bind_filter(tp, &prog->res, base);
>>          }
>>
>> If the classid is dynamic, why this tb[TCA_BPF_CLASSID]?
>
> The prog->res.classid is the default one, but can be overridden
> later depending on the specified program. cls_bpf_classify() does
> after prog return (filter_res holds return code):
>
>      [...]
>          if (filter_res == 0)
>              continue;
>          if (filter_res != -1) {
>              res->class   = 0;
>              res->classid = filter_res;
>          } else {
>              *res = prog->res;
>          }
>      [...]
>
> Meaning in case of a match (-1), we use the default bound one,
> but prog may as well return an alternative found classid if it
> wants to. So both versions are possible.

But even for that case your patch looks fine to me actually, since
for dynamic classid we set class to 0. No objections from my side
then.