From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-x236.google.com (mail-pf0-x236.google.com [IPv6:2607:f8b0:400e:c00::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id DBE5E3BA8E for ; Tue, 8 May 2018 00:12:20 -0400 (EDT) Received: by mail-pf0-x236.google.com with SMTP id a14so23332534pfi.1 for ; Mon, 07 May 2018 21:12:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=ByRKt6war2KSU5i1/FNE1q98PyxYpwJJUp/eqpjVI88=; b=RrfdUtCZ115n/ywFqbbyr+55sxX/wwDdughMVa3YoVz0vgNLyyCuvPmcA0wgVjzbPQ FtDzXOtZXfIDVhRCKu8AGAz6aAlK2YGc6rQeIdczXE/3VzI/pLZUbO/09+WpB6PQqPSh Djrwck8WohhKHJ3OIJUgj68xAq+ArSFWQn59eCZnyRALvYrvImBXxFRZ9MTrKWTIZ8UU MnAR0SDWfyMpVQSyh8rgOJhwVFB+o0Ca2F+Oudpctlc3GNeS8rxdlZaOlYryc1ue+zX7 /E71hJ+tXKSZFPq2/7iEuv8iON5JvzO38PgztnwmQvVbLIFrN/PRTWpmuWiYJUvdOKQG 4g6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=ByRKt6war2KSU5i1/FNE1q98PyxYpwJJUp/eqpjVI88=; b=inJoWyB40C1mY++7/zH5UCAgaSUcYU9Z3ORhjUsLAB+uaut0mXN886OsaV34Fc3Fr2 i4YyWL9JV4e7J6fw3h770t//y9gVIof8mfX3hvAVgI+FQf63k/YnjevawP68yuUQMaAG U5czJFuuxb0+P6N5N7slmCCX5O62Jh773unMadoXgPbRin6ADhUqwzjvVHdmN6A69UWr +T0hsQxG9Amh/yhBFVpdKH8AXvOtBA6vh+4vzELQk0ApM5DxfGl/0lKJv0V7ftVPkkL8 P0b70kbm8tpnKBy3NpV5wSwsufRFFIOUVhaITsfGpagU0KUorNjA4U55yQhqU5x3i3/F 18JQ== X-Gm-Message-State: ALQs6tD50cNsCJ/qxKJu7fLuGg8wnS2xPtpSkPE/9X4dy/u0BYihEmB/ 68NLd3AWNCYyYWXacbjh/ca81KR6J57joc3gowE= X-Google-Smtp-Source: AB8JxZpk0qxW3FZyW2BrFmEzxKf0RRS/KkzI9DztIBdDrwb7JdCk0gUrnW89WXrWaYCsMiIIg7SdNBl19dyqCq+X8O8= X-Received: by 2002:a63:6f46:: with SMTP id k67-v6mr30987291pgc.303.1525752740037; Mon, 07 May 2018 21:12:20 -0700 (PDT) MIME-Version: 1.0 Received: by 10.236.179.152 with HTTP; Mon, 7 May 2018 21:11:59 -0700 (PDT) In-Reply-To: <87in7zmhwy.fsf@toke.dk> References: <152544254217.11750.5727163821563013360.stgit@alrua-kau> <152544255298.11750.16512595539680113590.stgit@alrua-kau> <87in831bl2.fsf@toke.dk> <87in7zmhwy.fsf@toke.dk> From: Cong Wang Date: Mon, 7 May 2018 21:11:59 -0700 Message-ID: To: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= Cc: Linux Kernel Network Developers , Cake List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Tue, 08 May 2018 04:07:32 -0400 Subject: Re: [Cake] [PATCH net-next v8 1/7] sched: Add Common Applications Kept Enhanced (cake) qdisc X-BeenThere: cake@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: Cake - FQ_codel the next generation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 May 2018 04:12:21 -0000 On Mon, May 7, 2018 at 11:37 AM, Toke H=C3=B8iland-J=C3=B8rgensen wrote: > Cong Wang writes: > >> On Fri, May 4, 2018 at 12:10 PM, Toke H=C3=B8iland-J=C3=B8rgensen 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 =3D qdisc_priv(sch); >>>>> + int i, j; >>>>> + >>>>> + sch->limit =3D 10240; >>>>> + q->tin_mode =3D CAKE_DIFFSERV_BESTEFFORT; >>>>> + q->flow_mode =3D CAKE_FLOW_TRIPLE; >>>>> + >>>>> + q->rate_bps =3D 0; /* unlimited by default */ >>>>> + >>>>> + q->interval =3D 100000; /* 100ms default */ >>>>> + q->target =3D 5000; /* 5ms: codel RFC argues >>>>> + * for 5 to 10% of interval >>>>> + */ >>>>> + >>>>> + q->cur_tin =3D 0; >>>>> + q->cur_flow =3D 0; >>>>> + >>>>> + if (opt) { >>>>> + int err =3D 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 functio= n >>> 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!=3DNULL. > > 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().