netdev - RE: [NET-NEXT PATCH 2/3][UPDATED] pkt_sched: Add multiqueue scheduler support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 12 Sep 2008 16:33:45 -0700
From:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:	Jarek Poplawski <jarkao2@...il.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"jeff@...zik.org" <jeff@...zik.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: RE: [NET-NEXT PATCH 2/3][UPDATED] pkt_sched: Add multiqueue
	scheduler support

Jarek Poplawski wrote:
----------------------------------------------
>> +
>> +Currently two qdiscs support multiqueue devices.
>
> This looks like a bit misleading: one could think it's necessary to
> use one of these two with mq devices.
>
How about I change this from "support" to "are optimized for".  Would
that work better for you?

>> +
>> +sch_multiq has been added for hardware that wishes to avoid
>> +unnecessary requeuing.
>
> This could suggest that other qdiscs don't wish the same or even
> generate excessive requeuing, which IMHO could be true, but it's not
> proven, and might be only temporary situation anyway...
>
> Anyway, I wonder why you don't mention here anything from your first
> nice explantion to my doubts around the first attempt to restore
> multiqueue to prio, like EEDC/DCB/etc?

Actually this was just a poor choice of words on my part.  I should
probably replace "unnecessary requeueing" with "head-of-line blocking".


>> +Traffic will begin flowing through each queue if your base device
>> +has either the default simple_tx_hash or a custom
>> +netdev->select_queue() defined.
>
> I think a device doesn't need to have simple_tx_hash defined?

Actually simple_tx_hash is the default hash function that netdev uses
if select_queue is not defined.  Another poor choice of wording.  It
Should have been "..each queue via either the default simple_tx_hash
or the netdev->select_queue function if you have it defined".

>> +     for (i = q->bands; i < q->max_bands; i++) {
>> +             struct Qdisc *child = xchg(&q->queues[i], &noop_qdisc);
>> +             if (child != &noop_qdisc) {
>
> Or maybe?:
>                 if (q->queues[i] != &noop_qdisc) {
>                         struct Qdisc *child = xchg(&q->queues[i],
>                                                    &noop_qdisc);
>
>> +                     qdisc_tree_decrease_qlen(child, child->q.qlen);
>> +                     qdisc_destroy(child);
>> +             }
>> +     }
I suppose I can make this change since we are already protected by the
tree lock already.

>> +static int multiq_init(struct Qdisc *sch, struct nlattr *opt) +{
>> +     struct multiq_sched_data *q = qdisc_priv(sch); +     int i;
>> +
>> +     q->queues = NULL;
>> +
>> +     if (opt == NULL)
>> +             return -EINVAL;
>> +
>> +     q->max_bands = qdisc_dev(sch)->num_tx_queues; +
>> +     q->queues = kcalloc(q->max_bands, sizeof(struct Qdisc *),
...
>> +
>> +     return multiq_tune(sch, opt);
>
> But multiq_tune() can EINVAL, so kfree is needed here.

Noted and corrected.

> Jarek P.
>
> PS: to save some "paper" a typo, I guess, from PATCH 3/3:
>
>> +specific host, for example 192.168.0.3, though a specific queue
>> +you could use
>
>  +specific host, for example 192.168.0.3, through a specific queue
> you could use

Typo noted and will be corrected shortly.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html