[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4DA633C4.9080906@intel.com>
Date: Wed, 13 Apr 2011 16:37:40 -0700
From: John Fastabend <john.r.fastabend@...el.com>
To: Ben Hutchings <bhutchings@...arflare.com>
CC: "shemminger@...tta.com" <shemminger@...tta.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] iproute2: tc add mqprio qdisc support
On 4/13/2011 4:10 PM, Ben Hutchings wrote:
> I know that this has already been applied, but:
>
> On Tue, 2011-04-12 at 08:57 -0700, John Fastabend wrote:
>> Add mqprio qdisc support. Output matches the following,
>>
>> # ./tc/tc qdisc
>> qdisc mq 0: dev eth1 root
>> qdisc mq 0: dev eth2 root
>> qdisc mqprio 8001: dev eth3 root tc 8 map 0 1 2 3 4 5 6 7 1 1 1 1 1 1 1 1
>> queues:(0:7) (8:15) (16:23) (24:31) (32:39) (40:47) (48:55) (56:63)
>>
>> And usage is,
>>
>> # ./tc/tc qdisc add dev eth3 root mqprio help
>> Usage: ... mclass [num_tc NUMBER] [map P0 P1...]
>
> mclass?
agh stupid typo in the description that was my working name
for the qdisc some time ago. The help in 'tc' is correct.
>
>> [offset txq0 txq1 ...] [count cnt0 cnt1 ...] [hw 1|0]
>
> Of course I wrote something similar to this, but I never finished it
> off, so thanks.
>
> I don't think it makes sense to require count and offset to be specified
> as separate lists. The arguments could be interleaved but that adds
> more opportunity for error. Since offsets have to be in order and you
> generally don't want to have gaps then the offsets could normally be
> inferred. So maybe something like:
>
> queues cnt0[@txq0] cnt1[@txq1] ...
OK. I agree with you this is better.
>
> [...]
>> +static int mqprio_parse_opt(struct qdisc_util *qu, int argc,
>> + char **argv, struct nlmsghdr *n)
>> +{
>> + int idx;
>> + struct tc_mqprio_qopt opt = {
>> + 8,
>> + {0, 1, 2, 3, 4, 5, 6, 7, 0, 1, 1, 1, 3, 3, 3, 3},
>> + 1,
>> + };
>
> It would be clearer to name the fields being initialised.
>
OK.
> [...]
>> +int mqprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>> +{
>> + int i;
>> + struct tc_mqprio_qopt *qopt;
>> +
>> + if (opt == NULL)
>> + return 0;
>> +
>> + qopt = RTA_DATA(opt);
>> +
>> + fprintf(f, " tc %u map ", qopt->num_tc);
>> + for (i = 0; i <= TC_PRIO_MAX; i++)
>> + fprintf(f, "%d ", qopt->prio_tc_map[i]);
>> + fprintf(f, "\n queues:");
>> + for (i = 0; i < qopt->num_tc; i++)
>> + fprintf(f, "(%i:%i) ", qopt->offset[i],
>> + qopt->offset[i] + qopt->count[i] - 1);
> [...]
>
> Shouldn't this output be consistent with the command-line syntax?
I'm not sure, here's what it is now,
queues:(0:7) (8:15) (16:23) (24:31) (32:39) (40:47) (48:55) (56:63)
And here's what it would be with the change,
queues: 8@0 8@8 8@16 8@24 8@32 8@40 8@48 8@56
I like the first option with (#:#) it seems a bit more obvious to me
what the layout is. I'll get this fixed up tomorrow. Thanks for
taking a look Ben.
~John.
--
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
Powered by blists - more mailing lists