On 05/08/2018 10:24 PM, Cong Wang wrote:
On Tue, May 8, 2018 at 5:59 AM, Michel Machado <mic...@digirati.com.br> wrote:
Overall it looks good to me, just one thing below:
+struct Qdisc_ops gkprio_qdisc_ops __read_mostly = {
+ .id = "gkprio",
+ .priv_size = sizeof(struct gkprio_sched_data),
+ .enqueue = gkprio_enqueue,
+ .dequeue = gkprio_dequeue,
+ .peek = qdisc_peek_dequeued,
+ .init = gkprio_init,
+ .reset = gkprio_reset,
+ .change = gkprio_change,
+ .dump = gkprio_dump,
+ .destroy = gkprio_destroy,
+ .owner = THIS_MODULE,
+};
You probably want to add Qdisc_class_ops here so that you can
dump the stats of each internal queue.
Hi Cong,
In the production scenario we are targeting, this priority queue must be
classless; being classful would only bloat the code for us. I don't see
making this queue classful as a problem per se, but I suggest leaving it as
a future improvement for when someone can come up with a useful scenario for
it.
Take a look at sch_prio, it is fairly simple since your internal
queues are just an array... Per-queue stats are quite useful
in production, we definitely want to observe which queues are
full which are not.
DSprio cannot add Qdisc_class_ops without a rewrite of other queue
disciplines, which doesn't seem desirable. Since the method cops->leaf
is required (see register_qdisc()), we would need to replace the array
struct sk_buff_head qdiscs[GKPRIO_MAX_PRIORITY] in struct
gkprio_sched_data with the array struct Qdisc
*queues[GKPRIO_MAX_PRIORITY] to be able to return a Qdisc in
dsprio_leaf(). The problem with this change is that Qdisc does not have
a method to dequeue from its tail. This new method may not even make
sense in other queue disciplines. But without this method,
gkprio_enqueue() cannot drop the lowest priority packet when the queue
is full and an incoming packet has higher priority.
Nevertheless, I see your point on being able to observe the distribution
of queued packets per priority. A solution for that would be to add the
array __u32 qlen[GKPRIO_MAX_PRIORITY] in struct tc_gkprio_qopt. This
solution even avoids adding overhead in the critical paths of DSprio. Do
you see a better solution?
By the way, I've used GKPRIO_MAX_PRIORITY and other names that include
"gkprio" above to reflect the version 1 of this patch that we are
discussing. We will rename these identifiers for version 2 of this patch
to replace "gkprio" with "dsprio".
[ ]'s
Michel Machado
