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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpV4qThp2+_sKC7ZBgUCUu0BSZvOkT0tvf2ATdMFgu+YPg@mail.gmail.com>
Date:   Mon, 7 May 2018 09:35:30 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Toke Høiland-Jørgensen <toke@...e.dk>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Cake List <cake@...ts.bufferbloat.net>
Subject: Re: [PATCH net-next v8 1/7] sched: Add Common Applications Kept
 Enhanced (cake) qdisc

On Fri, May 4, 2018 at 12:10 PM, Toke Høiland-Jørgensen <toke@...e.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?

>
> - 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).


>>> +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.