[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20110106154115.GA1846@del.dom.local>
Date: Thu, 6 Jan 2011 16:41:15 +0100
From: Jarek Poplawski <jarkao2@...il.com>
To: John Fastabend <john.r.fastabend@...el.com>
Cc: "davem@...emloft.net" <davem@...emloft.net>,
"hadi@...erus.ca" <hadi@...erus.ca>,
"shemminger@...tta.com" <shemminger@...tta.com>,
"tgraf@...radead.org" <tgraf@...radead.org>,
"eric.dumazet@...il.com" <eric.dumazet@...il.com>,
"bhutchings@...arflare.com" <bhutchings@...arflare.com>,
"nhorman@...driver.com" <nhorman@...driver.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [net-next-2.6 PATCH v5 2/2] net_sched: implement a root
container qdisc sch_mqprio
On Wed, Jan 05, 2011 at 06:31:09PM -0800, John Fastabend wrote:
> On 1/5/2011 4:32 PM, Jarek Poplawski wrote:
> > On Wed, Jan 05, 2011 at 09:38:44AM -0800, John Fastabend wrote:
> >> On 1/4/2011 2:59 PM, Jarek Poplawski wrote:
> >>> On Tue, Jan 04, 2011 at 10:56:46AM -0800, John Fastabend wrote:
> > ...
> >>>> +
> >>>> + /* Always use supplied priority mappings */
> >>>> + for (i = 0; i < TC_BITMASK + 1; i++) {
> >>>> + if (netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i])) {
> >>>> + err = -EINVAL;
> >>>
> >>> This would probably trigger if we try qopt->num_tc == 0. Is it expected?
> >>
> >> netdev_set_prio_tc_map() returns 0 on sucess. This if(..) is a bit strange though.
> >>
> >> err = netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i])
> >> if (err)
> >> ...
> >>
> >> Is cleaner IMHO.
> >
> > Maybe. But I still wonder if qopt->num_tc == 0 is valid (by design)?
> > (netdev_set_prio_tc_map() returns -EINVAL if dev->num_tc == 0)
> >
>
> I guess I don't see how it returns -EINVAL. Although setting
> qopt->num_tc = 0 is equivalent to disabling traffic classes so
> it is a strange thing to do but I don't expect it should be a
> problem.
>
> static inline
> int netdev_set_prio_tc_map(struct net_device *dev, u8 prio, u8 tc)
> {
> if (tc >= dev->num_tc)
If dev->num_tc was set to 0 this check is always true and we exit
the init with error. It's not a problem if it's documented but
checking it earlier seems more readable.
> return -EINVAL;
>
> dev->prio_tc_map[prio & TC_BITMASK] = tc & TC_BITMASK;
> return 0;
> }
>
> >>>> +static unsigned long mqprio_get(struct Qdisc *sch, u32 classid)
> >>>> +{
> >>>> + unsigned int ntx = TC_H_MIN(classid);
> >>>
> >>> We need to 'get' tc classes too, eg for individual dumps. Then we
> >>> should omit them in .leaf, .graft etc.
> >>>
> >>
> >> OK missed this. Looks like iproute2 always sets NLM_F_DUMP which works because it uses cl_ops->walk
> >>
> >> # tc -s class show dev eth3 classid 800b:1
> >> class mqprio 800b:1 root
> >> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> >> backlog 0b 0p requeues 0
> >
> > OK, then it might be only of theoretical value (for some other tools).
> >>> Why dev->num_tc above, netdev_get_num_tc(dev) here, and dev->num_tc
> >>> below?
> >>
> >> No reason just inconsistant I will use dev->num_tc.
> >
> > Well, this would suggest netdev_get_num_tc() is redundant.
> >
>
> Guess it is I can remove it.
If you definitely don't expect any #ifdef CONFIG_XXX for this code,
both get and set num_tc could be removed. If you're not sure use the
function everywhere.
>
> static inline
> u8 netdev_get_num_tc(const struct net_device *dev)
> {
> return dev->num_tc;
> }
>
> >>>> +static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
> >>>> +{
> >>>> + struct net_device *dev = qdisc_dev(sch);
> >>>> + unsigned long ntx;
> >>>> + u8 num_tc = netdev_get_num_tc(dev);
> >>>> +
> >>>> + if (arg->stop)
> >>>> + return;
> >>>> +
> >>>> + /* Walk hierarchy with a virtual class per tc */
> >>>> + arg->count = arg->skip;
> >>>> + for (ntx = arg->skip; ntx < dev->num_tx_queues + num_tc; ntx++) {
> >>>
> >>> Should we report possibly unused/unconfigured tx_queues?
> >>
> >> I think it may be OK select_queue() could push skbs onto these queues and we may still want to see the statistics in this case. Although (real_num_tx_queues + num_tc) may make sense I see no reason to show queues above real_num_tx_queues.
> >
> > IMHO, pushing skbs to unconfigured/unwanted/disabled queues is a bug,
> > and select should handle this, but probably it's a matter of taste.
>
> certainly a bug for queues >= real_num_tx_queues.
>
> > Btw, I wonder how dev->real_num_tx_queues is obeyed here.
> >
>
> Its not. real_num_tx_queues is not handled correctly here also
> real_num_tx_queues can be changed so this need to be handled as well.
> This means an additional check in the options parsing.
Well, let's do it. Maybe similarly to sch_multiq, with the .change
option?
Jarek P.
--
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