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
