From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Subject: Re: [PATCH v3 net-next] net/sched: add skbprio scheduler Date: Mon, 9 Jul 2018 16:53:19 -0300 Message-ID: <20180709195319.GD8880@localhost.localdomain> References: <20180707101351.GA8300@gmail.com> <20180709154409.GC8880@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Nishanth Devarajan , xiyou.wangcong@gmail.com, jhs@mojatatu.com, jiri@resnulli.us, davem@davemloft.net, netdev@vger.kernel.org, doucette@bu.edu To: Michel Machado Return-path: Received: from mail-qt0-f181.google.com ([209.85.216.181]:38220 "EHLO mail-qt0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932847AbeGITxW (ORCPT ); Mon, 9 Jul 2018 15:53:22 -0400 Received: by mail-qt0-f181.google.com with SMTP id c5-v6so16408245qth.5 for ; Mon, 09 Jul 2018 12:53:22 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Jul 09, 2018 at 02:18:33PM -0400, Michel Machado wrote: > On 07/09/2018 11:44 AM, Marcelo Ricardo Leitner wrote: > > On Sat, Jul 07, 2018 at 03:43:55PM +0530, Nishanth Devarajan wrote: > > > net/sched: add skbprio scheduer > > > > > > Skbprio (SKB Priority Queue) is a queueing discipline that prioritizes packets > > > according to their skb->priority field. Under congestion, already-enqueued lower > > > priority packets will be dropped to make space available for higher priority > > > packets. Skbprio was conceived as a solution for denial-of-service defenses that > > > need to route packets with different priorities as a means to overcome DoS > > > attacks. > > > > Why can't we implement this as a new flag for sch_prio.c? > > > > I don't see why this duplication is needed, especially because it will > > only be "slower" (as in, it will do more work) when qdisc is already > > full and dropping packets anyway. > > sch_prio.c and skbprio diverge on a number of aspects: > > 1. sch_prio.c supports up to 16 priorities whereas skbprio 64. This is > not just a matter of changing a constant since sch_prio.c doesn't use > skb->priority. Yes it does use skb->priority for classifying into a band: prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) { struct prio_sched_data *q = qdisc_priv(sch); u32 band = skb->priority; ... > > 2. sch_prio.c does not have a global limit on the number of packets on > all its queues, only a limit per queue. It can be useful to sch_prio.c as well, why not? prio_enqueue() { ... + if (count > sch->global_limit) + prio_tail_drop(sch); /* to be implemented */ ret = qdisc_enqueue(skb, qdisc, to_free); > > 3. The queues of sch_prio.c are struct Qdisc, which don't have a method > to drop at its tail. That can be implemented, most likely as prio_tail_drop() as above. > > Given the divergences, adding flags to sch_prio.c will essentially keep > both implementations together instead of being isolated as being proposed. I don't agree. There aren't that many flags. I see only 2, one which makes sense to sch_prio as it is already (the global limit) and from where it should drop, the overflown packet or from tail. All other code will be reused: stats handling, netlink handling, enqueue and dequeue at least. If we add this other qdisc, named as it is, it will be very confusing to sysadmins: both are named very closely and work essentially in the same way, but one drops from tail and another drops the incoming packet. > > On the speed point, there may not be noticeable difference between both > qdiscs because the enqueueing and dequeueing costs of both qdics are O(1). > Notice that the "extra work" (i.e. dropping lower priority packets) is a key > aspect of skbprio since it gives routers a cheap way to choose which packets > to drop during a DoS. On that I agree. I was more referring to something like: "lets not make sch_prio slow and implement a new one instead.", which I don't it's valid because the extra "cost" is only visible when it's already dropping packets. Hopefully it's clearer now :) []s Marcelo