netdev - RE: [PATCH 3/3] NET: [SCHED] Qdisc changes and 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]

Message-ID: <D5C1322C3E673F459512FB59E0DDC3290314719B@orsmsx414.amr.corp.intel.com>
Date:	Thu, 21 Jun 2007 17:01:58 -0700
From:	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>
To:	"Patrick McHardy" <kaber@...sh.net>
Cc:	<davem@...emloft.net>, <netdev@...r.kernel.org>, <jeff@...zik.org>,
	"Kok, Auke-jan H" <auke-jan.h.kok@...el.com>, <hadi@...erus.ca>
Subject: RE: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

> The dependencies seem to be very confused. SCHED_PRIO does 
> not depend on anything new, SCH_RR also doesn't depend on 
> anything. SCH_PRIO_MQ and SCH_RR_MQ (which is missing) depend 
> on SCH_PRIO/SCH_RR. A single NET_SCH_MULTIQUEUE option seems 
> better than adding one per scheduler though.

I agree with a NET_SCH_MULTIQUEUE option.  However, SCH_RR does depend
on SCH_PRIO being built since it's the same code, doesn't it?  Maybe I'm
not understanding something about the build process.  I'll clean this
up.

> 
> > --- a/net/sched/sch_prio.c
> > +++ b/net/sched/sch_prio.c
> > @@ -9,6 +9,8 @@
> >   * Authors:	Alexey Kuznetsov, <kuznet@....inr.ac.ru>
> >   * Fixes:       19990609: J Hadi Salim <hadi@...telnetworks.com>:
> >   *              Init --  EINVAL when opt undefined
> > + * Additions:	Peter P. Waskiewicz Jr. 
> <peter.p.waskiewicz.jr@...el.com>
> > + *		Added round-robin scheduling for selection at load-time
> >   
> 
> git keeps changelogs, please don't add it here.

Roger.

> >  	struct tcf_proto *filter_list;
> >  	u8  prio2band[TC_PRIO_MAX+1];
> >  	struct Qdisc *queues[TCQ_PRIO_BANDS];
> > +	u16 band2queue[TC_PRIO_MAX + 1];
> >   
> 
> Why is this still here? Its a 1:1 mapping.

I'll fix this.

> > @@ -211,6 +265,22 @@ static int prio_tune(struct Qdisc 
> *sch, struct rtattr *opt)
> >  			return -EINVAL;
> >  	}
> >  
> > +	/* If we're prio multiqueue or are using round-robin, make
> > +	 * sure the number of incoming bands matches the number of
> > +	 * queues on the device we're associating with.
> > +	 */
> > +#ifdef CONFIG_NET_SCH_RR
> > +	if (strcmp("rr", sch->ops->id) == 0)
> > +		if (qopt->bands != sch->dev->egress_subqueue_count)
> > +			return -EINVAL;
> > +#endif
> > +
> > +#ifdef CONFIG_NET_SCH_PRIO_MQ
> > +	if (strcmp("prio", sch->ops->id) == 0)
> > +		if (qopt->bands != sch->dev->egress_subqueue_count)
> > +			return -EINVAL;
> > +#endif
> >   
> 
> For the tenth time now, the user should enable this at 
> runtime. You can't just break things dependant on config options.

I had this in sch_prio and tc before, and was told to remove it because
of ABI issues.  I can put it back in, but I'm not sure what those
previous ABI issues were.  Was it backwards compatibility that you
referred to before that was broken?

As always, the feedback is very much appreciated.  I'll get these fixes
in as soon as possible.

-PJ
-
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