net/sched: add skbprio scheduler

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.

Quoted 8 lines · Nishanth Devarajan

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.

Quoted 10 lines · Marcelo Ricardo Leitner

Quoted 8 lines · Nishanth Devarajan

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.

Quoted 5 lines

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.

2. sch_prio.c does not have a global limit on the number of packets on
all its queues, only a limit per queue.

3. The queues of sch_prio.c are struct Qdisc, which don't have a method
to drop at its tail.

Given the divergences, adding flags to sch_prio.c will essentially keep
both implementations together instead of being isolated as being proposed.

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.

Quoted 17 lines · Michel Machado

Quoted 10 lines · Marcelo Ricardo Leitner

Quoted 8 lines · Nishanth Devarajan

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.

Quoted 5 lines

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.

Quoted 5 lines

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;

...

Quoted 2 lines

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.

Quoted 26 lines · Marcelo Ricardo Leitner

Quoted 17 lines · Michel Machado

Quoted 10 lines · Marcelo Ricardo Leitner

Quoted 8 lines · Nishanth Devarajan

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.

Quoted 5 lines

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.

Quoted 5 lines

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:

Quoted 5 lines

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.

Quoted 5 lines

Quoted 3 lines

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

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.

Quoted 27 lines · Michel Machado

Quoted 26 lines · Marcelo Ricardo Leitner

Quoted 17 lines · Michel Machado

Quoted 10 lines · Marcelo Ricardo Leitner

Quoted 8 lines · Nishanth Devarajan

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.

Quoted 5 lines

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.

Quoted 5 lines

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:

Quoted 8 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

A solution would be to add another flag to switch between the current
prio_classify() and a new one to just use skb->priority as in skbprio. This

way we don't risk breaking applications that rely on tcf_classify() and this odd behavior that I found in prio_classify():

        band = TC_H_MIN(band) - 1;
        if (band >= q->bands)
                return q->queues[q->prio2band[0]];
        return q->queues[band];

When band is zero, it returns q->queues[q->prio2band[0]] instead of
q->queues[band] as it would for other bands less than q->bands.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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

If it is not implemented for all available qdiscs and the flag to drop at
the tail is on, sch_prio.c would need to issue a log message whenever a packet goes into one of the subqueues that don't drop at the tail and have a failsafe behavior.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 1 line

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

We need to choose between (1) having skbprio that has some duplicate code
with sch_prio.c and (2) adding flags to sch_prio.c and make a major refactoring of the schedule subsystem to add drop an the tail to qdiscs.

I mean major because we are not just talking about adding the method
dequeue_tail() to struct Qdisc and adding dequeue_tail() to all qdiscs. One

will need to come up with definitions of dequeue_tail() for qdiscs that don't naturally have it and even rewrite the data structures of qdiscs. To substantiate this last point, consider sch_fifo.c, one of the simplest qdiscs available. sch_fifo.c keeps its packets in sch->q, which is of type struct qdisc_skb_head. struct qdisc_skb_head doesn't set skb->prev, so it cannot drop at the tail without walking through its list.

I do understand the motivation for minimizing duplicate code. But the
small amount of duplicate code that skbprio adds is cheaper than refactoring the scheduler system to only support this new sch_prio.c.

Quoted 2 lines · Michel Machado

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.

Quoted 8 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Michel Machado

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.

struct tc_prio_qopt {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
};

How would you do it carefully?

Also, it is not only used by prio but also pfifo_fast.

Quoted 11 lines · Cong Wang

Quoted 8 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Michel Machado

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.

Quoted 6 lines

struct tc_prio_qopt {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
};

How would you do it carefully?

quick shot, multiplex v1 and v2 formats based on bands and sizeof():

#define TCQ_PRIO_BANDS_V1       16
#define TCQ_PRIO_BANDS_V2       64
#define TC_PRIO_MAX_V2          64

struct tc_prio_qopt_v2 {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX_V2+1]; /* Map: logical priority -> PRIO band */
};

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 11 lines · Cong Wang

Quoted 8 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Michel Machado

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.

Quoted 6 lines

struct tc_prio_qopt {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
};

How would you do it carefully?

quick shot, multiplex v1 and v2 formats based on bands and sizeof():

Quoted 9 lines

#define TCQ_PRIO_BANDS_V1       16
#define TCQ_PRIO_BANDS_V2       64
#define TC_PRIO_MAX_V2          64

struct tc_prio_qopt_v2 {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX_V2+1]; /* Map: logical priority -> PRIO band */
};

Good try, but:

1. You don't take padding into account, although the difference between 16 and 64 is big here. If it were 16 and 20, almost certainly wouldn't work.

2. What if I compile a new iproute2 on an old kernel? The iproute2 will use V2, while old kernel has no knowledge of V2, so it only copies a part of V2 in the end....

Quoted 34 lines · Cong Wang

Quoted 31 lines · Marcelo Ricardo Leitner

Quoted 11 lines · Cong Wang

Quoted 8 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Michel Machado

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.

Quoted 6 lines

struct tc_prio_qopt {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
};

How would you do it carefully?

quick shot, multiplex v1 and v2 formats based on bands and sizeof():

#define TCQ_PRIO_BANDS_V1       16
#define TCQ_PRIO_BANDS_V2       64
#define TC_PRIO_MAX_V2          64

struct tc_prio_qopt_v2 {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX_V2+1]; /* Map: logical priority -> PRIO band */
};

Quoted 5 lines

Good try, but:

1. You don't take padding into account, although the difference between 16 and 64 is big here. If it were 16 and 20, almost certainly wouldn't work.

It still would work, no matter how much padding you have, as currently you can't use more than 3 bands.

Quoted 5 lines

2. What if I compile a new iproute2 on an old kernel? The iproute2 will use V2, while old kernel has no knowledge of V2, so it only copies a part of V2 in the end....

Yes, and that's not a problem:
- Either bands is > 3 and it will return EINVAL, protecting from reading beyond the buffer.
- Or 2 <= bands <= 3 and it will handle it as a _v1 struct, and use only the original size.

iproute2 (or other app) may still use _v1 if it wants, btw.

Quoted 42 lines · Marcelo Ricardo Leitner

Quoted 39 lines · Cong Wang

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 11 lines · Cong Wang

Quoted 8 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Michel Machado

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.

Quoted 6 lines

struct tc_prio_qopt {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
};

How would you do it carefully?

quick shot, multiplex v1 and v2 formats based on bands and sizeof():

Quoted 9 lines

#define TCQ_PRIO_BANDS_V1       16
#define TCQ_PRIO_BANDS_V2       64
#define TC_PRIO_MAX_V2          64

struct tc_prio_qopt_v2 {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX_V2+1]; /* Map: logical priority -> PRIO band */
};

Good try, but:

1. You don't take padding into account, although the difference between 16 and 64 is big here. If it were 16 and 20, almost certainly wouldn't work.

Quoted 2 lines

It still would work, no matter how much padding you have, as currently you can't use more than 3 bands.

I am lost.

With your proposal above, you have 16 bands for V1 and 64 bands for V2, where does 3 come from???

Quoted 8 lines

Quoted 7 lines

2. What if I compile a new iproute2 on an old kernel? The iproute2 will use V2, while old kernel has no knowledge of V2, so it only copies a part of V2 in the end....

Quoted 5 lines

Yes, and that's not a problem:
- Either bands is > 3 and it will return EINVAL, protecting from reading beyond the buffer.
- Or 2 <= bands <= 3 and it will handle it as a _v1 struct, and use only the original size.

Again why 3 not 16 or 64 ??

Also, why does an old kernel has the logic in its binary to determine this?

Quoted 3 lines

iproute2 (or other app) may still use _v1 if it wants, btw.

Yes, old iproute2 must still have v1, what's point? Are you

suggesting new iproute2 should still have v1 after you propose v1 and v2 for kernel?

I must seriously miss something. Please help.

Thanks!

Quoted 47 lines · Cong Wang

Quoted 44 lines · Marcelo Ricardo Leitner

Quoted 34 lines · Cong Wang

Quoted 22 lines · Marcelo Ricardo Leitner

Quoted 11 lines · Cong Wang

Quoted 8 lines · Marcelo Ricardo Leitner

Quoted 2 lines · Michel Machado

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.

Quoted 6 lines

struct tc_prio_qopt {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
};

How would you do it carefully?

quick shot, multiplex v1 and v2 formats based on bands and sizeof():

Quoted 9 lines

#define TCQ_PRIO_BANDS_V1       16
#define TCQ_PRIO_BANDS_V2       64
#define TC_PRIO_MAX_V2          64

struct tc_prio_qopt_v2 {
int bands; /* Number of bands */
__u8 priomap[TC_PRIO_MAX_V2+1]; /* Map: logical priority -> PRIO band */
};

Quoted 5 lines

Good try, but:

1. You don't take padding into account, although the difference between 16 and 64 is big here. If it were 16 and 20, almost certainly wouldn't work.

It still would work, no matter how much padding you have, as currently you can't use more than 3 bands.

Quoted 4 lines

I am lost.

With your proposal above, you have 16 bands for V1 and 64 bands for V2, where does 3 come from???

My bad. s/3/16/

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

2. What if I compile a new iproute2 on an old kernel? The iproute2 will use V2, while old kernel has no knowledge of V2, so it only copies a part of V2 in the end....

Quoted 7 lines

Quoted 5 lines

Yes, and that's not a problem:
- Either bands is > 3 and it will return EINVAL, protecting from reading beyond the buffer.
- Or 2 <= bands <= 3 and it will handle it as a _v1 struct, and use only the original size.

Again why 3 not 16 or 64 ??

Again, s/3/16/

Quoted 3 lines

Also, why does an old kernel has the logic in its binary to determine this?

It won't, and it doesn't need to. If you use bands > 16 with an old kernel, it will reject per current code (that I already pasted):

        if (qopt->bands > TCQ_PRIO_BANDS || qopt->bands < 2)
                return -EINVAL;

Simple as that. If you try to use more bands than it supports, it will reject it.

Quoted 5 lines

Quoted 4 lines

iproute2 (or other app) may still use _v1 if it wants, btw.

Quoted 1 line

Yes, old iproute2 must still have v1, what's point? Are you

??

Quoted 2 lines

suggesting new iproute2 should still have v1 after you propose v1 and v2 for kernel?

I'm only saying that both versions will be accepted by a new kernel.

Quoted 3 lines · Michel Machado

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()
{
...

Quoted 11 lines · Marcelo Ricardo Leitner

Quoted 3 lines · Michel Machado

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()
{
...

Review

Quoted 3 lines

(quoted diff) +2 -0

+       if (count > sch->global_limit)
+               prio_tail_drop(sch);   /* to be implemented */
        ret = qdisc_enqueue(skb, qdisc, to_free);

Isn't the whole point of sch_prio offloading the queueing to each class? If you need a limit, there is one for each child qdisc if you use for example pfifo or bfifo (depending on you want to limit bytes or packets).

Also, what's your plan for backward compatibility here?

Quoted 17 lines · Cong Wang

Quoted 11 lines · Marcelo Ricardo Leitner

Quoted 3 lines · Michel Machado

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()
{
...

Review

Quoted 3 lines

(quoted diff) +2 -0

+       if (count > sch->global_limit)
+               prio_tail_drop(sch);   /* to be implemented */
        ret = qdisc_enqueue(skb, qdisc, to_free);

Quoted 4 lines

Isn't the whole point of sch_prio offloading the queueing to each class? If you need a limit, there is one for each child qdisc if you use for example pfifo or bfifo (depending on you want to limit bytes or packets).

Yes, but Michel wants to drop from other lower priorities if needed, and that's not possible if you handle the limit already in a child qdisc as they don't know about their siblings. The idea in the example above is to discard it from whatever lower priority is needed, then queue it. (ok, the example missed to check the priority level)

As for the different units, sch_prio holds a count of how many packets are queued on its children, and that's what would be used for the limit.

Quoted 2 lines

Also, what's your plan for backward compatibility here?

say:
if (sch->global_limit && count > sch->global_limit)
as in, only do the limit check/enforcing if needed.

Quoted 24 lines · Marcelo Ricardo Leitner

Quoted 21 lines · Cong Wang

Quoted 11 lines · Marcelo Ricardo Leitner

Quoted 3 lines · Michel Machado

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()
{
...

Review

Quoted 3 lines

(quoted diff) +2 -0

+       if (count > sch->global_limit)
+               prio_tail_drop(sch);   /* to be implemented */
        ret = qdisc_enqueue(skb, qdisc, to_free);

Isn't the whole point of sch_prio offloading the queueing to each class? If you need a limit, there is one for each child qdisc if you use for example pfifo or bfifo (depending on you want to limit bytes or packets).

Quoted 5 lines

Yes, but Michel wants to drop from other lower priorities if needed, and that's not possible if you handle the limit already in a child qdisc as they don't know about their siblings. The idea in the example above is to discard it from whatever lower priority is needed, then queue it. (ok, the example missed to check the priority level)

So it disproves your point of adding a flag to sch_prio, right?

Also, you have to re-introduce qdisc->ops->drop() if you really want to go this direction.

Quoted 6 lines

As for the different units, sch_prio holds a count of how many packets are queued on its children, and that's what would be used for the limit.

Quoted 1 line

Quoted 5 lines

Quoted 1 line

Also, what's your plan for backward compatibility here?

say:
if (sch->global_limit && count > sch->global_limit)
as in, only do the limit check/enforcing if needed.

Obviously doesn't work, users could pass 0 to effectively disable the qdisc from enqueue'ing any packet.

Quoted 32 lines · Cong Wang

Quoted 29 lines · Marcelo Ricardo Leitner

Quoted 17 lines · Cong Wang

Quoted 11 lines · Marcelo Ricardo Leitner

Quoted 3 lines · Michel Machado

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()
{
...

Review

Quoted 3 lines

(quoted diff) +2 -0

+       if (count > sch->global_limit)
+               prio_tail_drop(sch);   /* to be implemented */
        ret = qdisc_enqueue(skb, qdisc, to_free);

Quoted 4 lines

Isn't the whole point of sch_prio offloading the queueing to each class? If you need a limit, there is one for each child qdisc if you use for example pfifo or bfifo (depending on you want to limit bytes or packets).

Yes, but Michel wants to drop from other lower priorities if needed, and that's not possible if you handle the limit already in a child qdisc as they don't know about their siblings. The idea in the example above is to discard it from whatever lower priority is needed, then queue it. (ok, the example missed to check the priority level)

Quoted 1 line

So it disproves your point of adding a flag to sch_prio, right?

I don't see how?

Quoted 4 lines

Also, you have to re-introduce qdisc->ops->drop() if you really want to go this direction.

Again, yes. What's the deal with it?

Quoted 12 lines

Quoted 11 lines

As for the different units, sch_prio holds a count of how many packets are queued on its children, and that's what would be used for the limit.

Quoted 2 lines

Also, what's your plan for backward compatibility here?

say:
if (sch->global_limit && count > sch->global_limit)
as in, only do the limit check/enforcing if needed.

Quoted 2 lines

Obviously doesn't work, users could pass 0 to effectively disable the qdisc from enqueue'ing any packet.

If you only had considered the right 'limit' variable, you would be right here.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

Isn't the whole point of sch_prio offloading the queueing to each class? If you need a limit, there is one for each child qdisc if you use for example pfifo or bfifo (depending on you want to limit bytes or packets).

Yes, but Michel wants to drop from other lower priorities if needed, and that's not possible if you handle the limit already in a child qdisc as they don't know about their siblings. The idea in the example above is to discard it from whatever lower priority is needed, then queue it. (ok, the example missed to check the priority level)

Quoted 3 lines

Quoted 1 line

So it disproves your point of adding a flag to sch_prio, right?

I don't see how?

Interesting, you said "Michel wants to drop from other lower priorities if needed", but sch_prio has no knowledge of this, you confirmed with "...if you handle the limit already in a child qdisc as they don't know about their siblings."

The if clause is true as the limit is indeed handled by its child qdiscs as designed.

Therefore, a simple of adding a flag to sch_prio, as you suggested and demonstrated above, doesn't work, as confirmed by your own words.

What am I missing here?

Are you go further by suggesting moving the limit out of prio? Or are you going to expand your definition of "adding a flag"? Perhaps two flags? :)

I am very open for discussion to see how far we can go.

Well, it would help if you didn't cut out key parts of my words.

Quoted 4 lines

What am I missing here?

Quoted 5 lines

Are you go further by suggesting moving the limit out of prio? Or are you going to expand your definition of "adding a flag"? Perhaps two flags? :)

I am very open for discussion to see how far we can go.

I am not keen on continuing this discussion if you keep twisting my words just for fun.

3. The queues of sch_prio.c are struct Qdisc, which don't have a
method to drop at its tail.

Documentation/networking/sch_skbprio.txt

Quoted 5 lines · Nishanth Devarajan

Documentation/networking/sch_skbprio.txt new file +0 -0

new file mode 100644
index 0000000..3aa4d3e

We usually document each qdisc behavior in tc man pages in iproute2.

I don't mind you document it in kernel, but it kinda breaks the tradition.

Quoted 10 lines

Documentation/networking/sch_skbprio.txt new file +10 -0

+static int skbprio_change(struct Qdisc *sch, struct nlattr *opt,
+                       struct netlink_ext_ack *extack)
+{
+       struct skbprio_sched_data *q = qdisc_priv(sch);
+       struct tc_skbprio_qopt *ctl = nla_data(opt);
+       const unsigned int min_limit = 1;
+
+       if (ctl->limit == (typeof(ctl->limit))-1)
+               sch->limit = max(qdisc_dev(sch)->tx_queue_len, min_limit);
+       else if (ctl->limit < min_limit)

Documentation/networking/sch_skbprio.txt

Quoted 3 lines

Documentation/networking/sch_skbprio.txt new file +3 -0

+               return -EINVAL;
+       else
+               sch->limit = ctl->limit;

The dev->tx_queue_len is fundamentally non-sense since now almost every real NIC is multi-queue and qdisc has a completely different sch->limit. This is why I suggested you to simply avoid it in your code.

There is no standard way to use dev->tx_queue_len in kernel, so I can't claim your use is correct or not, but it still looks odd, other qdisc seems just uses as a default, rather than picking the smaller or bigger value as a cap.

Quoted 4 lines · Cong Wang

The dev->tx_queue_len is fundamentally non-sense since now almost every real NIC is multi-queue and qdisc has a completely different sch->limit. This is why I suggested you to simply avoid it in your code.

Would you be okay with a constant there? If so, we could just put 64
there. The optimal number is hardware dependent, but we don't know how to calculate it.

Quoted 5 lines

There is no standard way to use dev->tx_queue_len in kernel, so I can't claim your use is correct or not, but it still looks odd, other qdisc seems just uses as a default, rather than picking the smaller or bigger value as a cap.

    The reason for the `max(qdisc_dev(sch)->tx_queue_len, min_limit)` is

to make sure that sch->limit is at least 1. We couldn't come up with a meaningful behavior for sch->limit being zero, so we defined the basis case of skbprio_enqueue() as sch->limit one. If there's a guarantee that qdisc_dev(sch)->tx_queue_len is always greater than zero, we don't need the max().