Re: [Cake] [PATCH net-next v8 1/7] sched: Add Common Applications Kept Enhanced (cake) qdisc

[Cong Wang <xiyou.wangcong@gmail.com>]

On Mon, May 7, 2018 at 11:37 AM, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> writes:
>
>> On Fri, May 4, 2018 at 12:10 PM, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> Thank you for the review! A few comments below, I'll fix the rest.
>>>
>>>> [...]
>>>>
>>>> So sch_cake doesn't accept normal tc filters? Is this intentional?
>>>> If so, why?
>>>
>>> For two reasons:
>>>
>>> - The two-level scheduling used in CAKE (tins / diffserv classes, and
>>>   flow hashing) does not map in an obvious way to the classification
>>>   index of tc filters.
>>
>> Sounds like you need to extend struct tcf_result?
>
> Well, the obvious way to support filters would be to have skb->priority
> override the diffserv mapping if set, and have the filter classification
> result select the queue within that tier. That would probably be doable,
> but see below.
>
>>> - No one has asked for it. We have done our best to accommodate the
>>>   features people want in a home router qdisc directly in CAKE, and the
>>>   ability to integrate tc filters has never been requested.
>>
>> It is not hard to integrate, basically you need to call
>> tcf_classify(). Although it is not mandatory, it is odd to merge a
>> qdisc doesn't work with existing tc filters (and actions too).
>
> I looked at the fq_codel code to do this. Is it possible to support
> filtering without implementing Qdisc_class_ops? If so, I'll give it a
> shot; but implementing the class ops is more than I can commit to...

Good question. The tc classes in flow-based qdisc's are actually
used as flows rather than a normal tc class in a hierarchy qdisc.
Like in fq_code, the classes are mapped to each flow and because
of that we can dump stats of each flow.

I am not sure if you can totally bypass class_ops, you need to look
into these API's. Most of them are easy to implement, probably
only except the ->dump_stats(), so I don't think it is a barrier here.


>
>>>>> +static int cake_init(struct Qdisc *sch, struct nlattr *opt,
>>>>> +                    struct netlink_ext_ack *extack)
>>>>> +{
>>>>> +       struct cake_sched_data *q = qdisc_priv(sch);
>>>>> +       int i, j;
>>>>> +
>>>>> +       sch->limit = 10240;
>>>>> +       q->tin_mode = CAKE_DIFFSERV_BESTEFFORT;
>>>>> +       q->flow_mode  = CAKE_FLOW_TRIPLE;
>>>>> +
>>>>> +       q->rate_bps = 0; /* unlimited by default */
>>>>> +
>>>>> +       q->interval = 100000; /* 100ms default */
>>>>> +       q->target   =   5000; /* 5ms: codel RFC argues
>>>>> +                              * for 5 to 10% of interval
>>>>> +                              */
>>>>> +
>>>>> +       q->cur_tin = 0;
>>>>> +       q->cur_flow  = 0;
>>>>> +
>>>>> +       if (opt) {
>>>>> +               int err = cake_change(sch, opt, extack);
>>>>> +
>>>>> +               if (err)
>>>>> +                       return err;
>>>>
>>>>
>>>> Not sure if you really want to reallocate q->tines below for this
>>>> case.
>>>
>>> I'm not sure what you mean here? If there's an error we return it and
>>> the qdisc is not created. If there's not, we allocate and on subsequent
>>> changes cake_change() will be called directly, or? Can the init function
>>> ever be called again during the lifetime of the qdisc?
>>>
>>
>> In non-error case, you call cake_change() first and then allocate
>> ->tins with kvzalloc() below. For me it looks like you don't need to
>> allocate it again when ->tins!=NULL.
>
> No, we definitely don't. It's just not clear to me how cake_init() could
> ever be called with q->tins already allocated?
>
> I can add a check in any case, though, I see that there is one in
> fq_codel as well...

Ah, that's right, you have a check in cake_change() before
cake_reconfigure().