netdev - Re: [net-next-2.6 PATCH v5 2/2] net_sched: implement a root container qdisc sch

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 6 Jan 2011 01:32:08 +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 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)

> >> +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.

> >> +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.
Btw, I wonder how dev->real_num_tx_queues is obeyed here.

Thanks,
Jarek P.

PS: No offense, but could you try cutting lines ~70c. It's strongly
recommended on kernel lists.
--
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