On Mon, May 7, 2018 at 11:37 AM, Toke Høiland-Jørgensen <t...@toke.dk> wrote:
> Cong Wang <xiyou.wangc...@gmail.com> writes:
>
>> On Fri, May 4, 2018 at 12:10 PM, Toke Høiland-Jørgensen <t...@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().
