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

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

On Mon, Jul 09, 2018 at 05:03:31PM -0400, Michel Machado wrote:
> On 07/09/2018 03:53 PM, Marcelo Ricardo Leitner wrote:
> > 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;
> > ...
> 
>    Changing TC_PRIO_MAX from 15 to 63 risks breaking backward compatibility
> with applications.

If done, it needs to be done carefully, indeed. I don't know if it's
doable, neither I know how hard is your requirement for 64 different
priorities.

You can get 64 different priorities by stacking sch_prio, btw. And if
you implement drop_from_tail() as part of Qdisc, you can even get it
working for this cascading case too.

> 
> > >     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.
> 
>    struct Qdisc represents *all* qdiscs. My knowledge of the other qdiscs is
> limited, but not all qdiscs may have a meaningful method to drop at the
> tail. For example: a qdisc that works over flows may not know with flow is

True, but it doesn't mean you have to implement it for all available qdiscs.

> the tail. Not to mention that this would be a widespread patch to only
> support this new prio qdisc. It would be prudent to wait for the production
> success of the proposed, self-contained qdisc before making this commitment.

On the other hand, by adding another qdisc you're adding more work
that one needs to do when dealing with qdisc infrastructure, such as
updating enqueue() prototype, for example.

Once this new qdisc is in, it won't be easy to deprecate it.

  Marcelo