On 07/10/2018 10:57 PM, Cong Wang wrote:
On Sat, Jul 7, 2018 at 3:14 AM Nishanth Devarajan <ndev2...@gmail.com> wrote:
diff --git a/Documentation/networking/sch_skbprio.txt
b/Documentation/networking/sch_skbprio.txt
new file mode 100644
index 0000000..3aa4d3e
--- /dev/null
+++ b/Documentation/networking/sch_skbprio.txt
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.
We're going to move this text to tc man pages for v4.
+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)
+ 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.
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.
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().
[ ]'s
Michel Machado
