Re: [PATCH v3 net-next] net/sched: add skbprio scheduler

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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