From: Patrick McHardy <kaber@trash•net>
To: Jarek Poplawski <jarkao2@gmail•com>
Cc: netdev@vger•kernel.org
Subject: Re: net_sched 07/07: add classful multiqueue dummy scheduler
Date: Mon, 07 Sep 2009 15:27:37 +0200 [thread overview]
Message-ID: <4AA50A49.7010905@trash.net> (raw)
In-Reply-To: <20090906200409.GB8833@ami.dom.local>
Jarek Poplawski wrote:
>> struct Qdisc_class_ops
>> {
>> /* Child qdisc manipulation */
>> + unsigned int (*select_queue)(struct Qdisc *, struct tcmsg *);
>> int (*graft)(struct Qdisc *, unsigned long cl,
>> struct Qdisc *, struct Qdisc **);
>> struct Qdisc * (*leaf)(struct Qdisc *, unsigned long cl);
>> @@ -122,6 +123,7 @@ struct Qdisc_ops
>> void (*reset)(struct Qdisc *);
>> void (*destroy)(struct Qdisc *);
>> int (*change)(struct Qdisc *, struct nlattr *arg);
>> + void (*attach)(struct Qdisc *);
>
> Probably it's a matter of taste, but I wonder why these two methods
> used only by one qdisc in max 2 places can't be functions instead
> (maybe even static in case of select_queue)? (And this mq sched could
> be tested with some flag instead of ->attach, I guess.)
Yes, we could also use normal functions. Either way is fine with me.
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index d71f12b..2a78d54 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -678,6 +678,11 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>> if (dev->flags & IFF_UP)
>> dev_deactivate(dev);
>>
>> + if (new && new->ops->attach) {
>> + new->ops->attach(new);
>> + num_q = 0;
>> + }
>> +
>
> Actually, I wonder if it's not cleaner to let replace all qdiscs with
> noops below like in qdisc delete case, and do this attaching in one
> place only (dev_activate).
I don't think that would work since dev_activate() allocates its own
qdiscs, which use different handles than those specified by userspace.
We also need the new qdisc for notifications. It would be a nice
cleanup however if you can make it work.
>> @@ -1095,10 +1100,16 @@ create_n_graft:
>> q = qdisc_create(dev, &dev->rx_queue,
>> tcm->tcm_parent, tcm->tcm_parent,
>> tca, &err);
>> - else
>> - q = qdisc_create(dev, netdev_get_tx_queue(dev, 0),
>> + else {
>> + unsigned int ntx = 0;
>> +
>> + if (p && p->ops->cl_ops && p->ops->cl_ops->select_queue)
>> + ntx = p->ops->cl_ops->select_queue(p, tcm);
>
> So, this if could be probably made shorter with a common function, but
> the main point is: this probably works only for qdiscs having mq as a
> parent, and not below.
Yes. mq can only be attached to the root however, so its not
possible to use it as a child qdisc.
>> +static int mq_init(struct Qdisc *sch, struct nlattr *opt)
>> +{
>> + struct net_device *dev = qdisc_dev(sch);
>> + struct mq_sched *priv = qdisc_priv(sch);
>> + struct netdev_queue *dev_queue;
>> + struct Qdisc *qdisc;
>> + unsigned int ntx;
>> +
>> + if (sch->parent != TC_H_ROOT)
>> + return -EOPNOTSUPP;
>> +
>> + if (!netif_is_multiqueue(dev))
>> + return -EOPNOTSUPP;
>> +
>> + /* pre-allocate qdiscs, attachment can't fail */
>> + priv->qdiscs = kcalloc(dev->num_tx_queues, sizeof(priv->qdiscs[0]),
>> + GFP_KERNEL);
>
> I guess we could avoid this at all or at least to do it in one step with
> current ->attach.
It seemed easier this way, but I don't care much where its done exactly.
>> + if (priv->qdiscs == NULL)
>> + return -ENOMEM;
>> +
>> + for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
>> + dev_queue = netdev_get_tx_queue(dev, ntx);
>> + qdisc = qdisc_create_dflt(dev, dev_queue, &pfifo_fast_ops,
>> + TC_H_MAKE(TC_H_MAJ(sch->handle),
>> + TC_H_MIN(ntx + 1)));
>
> As I wrote in 05/07 comment, I wonder if we really can't achieve this
> with old TC_H_ROOT parentid, and maybe some mapping while dumping to
> the userspace only.
I don't see the advantage.
> Another possibility would be considering a new
> kind of root (mqroot?) to tell precisely, where a new qdisc should be
> added.
That's what mq is doing.
>> +static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
>> +{
>> + struct net_device *dev = qdisc_dev(sch);
>> + struct Qdisc *qdisc;
>> + unsigned int ntx;
>> +
>> + sch->q.qlen = 0;
>> + memset(&sch->bstats, 0, sizeof(sch->bstats));
>> + memset(&sch->qstats, 0, sizeof(sch->qstats));
>> +
>> + for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
>> + qdisc = netdev_get_tx_queue(dev, ntx)->qdisc_sleeping;
>> + spin_lock_bh(qdisc_lock(qdisc));
>> + sch->q.qlen += qdisc->q.qlen;
>> + sch->bstats.bytes += qdisc->bstats.bytes;
>> + sch->bstats.packets += qdisc->bstats.packets;
>> + sch->qstats.qlen += qdisc->qstats.qlen;
>
> Like in Christoph's case, we should probably use q.qlen instead.
Its done a few lines above. This simply sums up all members of qstats.
next prev parent reply other threads:[~2009-09-07 13:27 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-09-04 16:41 net_sched 00/07: classful multiqueue dummy scheduler Patrick McHardy
2009-09-04 16:41 ` net_sched 01/07: fix class grafting errno codes Patrick McHardy
2009-09-04 16:41 ` net_sched 02/07: make cls_ops->tcf_chain() optional Patrick McHardy
2009-09-05 8:13 ` Jarek Poplawski
2009-09-05 11:57 ` Jarek Poplawski
2009-09-05 12:32 ` Jarek Poplawski
2009-09-05 17:03 ` Patrick McHardy
2009-09-06 9:06 ` David Miller
2009-09-04 16:41 ` net_sched 03/07: make cls_ops->change and cls_ops->delete optional Patrick McHardy
2009-09-04 16:41 ` net_sched 04/07: remove some unnecessary checks in classful schedulers Patrick McHardy
2009-09-04 16:41 ` net_sched 05/07: reintroduce dev->qdisc for use by sch_api Patrick McHardy
2009-09-06 18:57 ` Jarek Poplawski
2009-09-07 13:16 ` Patrick McHardy
2009-09-07 16:49 ` Jarek Poplawski
2009-09-04 16:41 ` net_sched 06/07: move dev_graft_qdisc() to sch_generic.c Patrick McHardy
2009-09-04 16:41 ` net_sched 07/07: add classful multiqueue dummy scheduler Patrick McHardy
2009-09-06 20:04 ` Jarek Poplawski
2009-09-07 13:27 ` Patrick McHardy [this message]
2009-09-07 18:22 ` Jarek Poplawski
2009-09-07 19:24 ` Jarek Poplawski
2009-09-07 19:49 ` Eric Dumazet
2009-09-09 16:02 ` Patrick McHardy
2009-09-09 19:52 ` Jarek Poplawski
2009-09-10 11:28 ` Patrick McHardy
2009-09-11 21:38 ` Jarek Poplawski
2009-09-11 22:10 ` David Miller
2009-09-11 22:21 ` Jarek Poplawski
2009-09-11 22:27 ` David Miller
2009-09-09 16:01 ` Patrick McHardy
2009-09-04 16:42 ` net_sched 00/07: " Patrick McHardy
2009-09-07 8:50 ` David Miller
2009-09-07 9:46 ` Jarek Poplawski
2009-09-07 13:00 ` Eric Dumazet
2009-09-07 13:29 ` Patrick McHardy
2009-09-07 14:23 ` Patrick McHardy
2009-09-07 17:21 ` Eric Dumazet
2009-09-07 17:28 ` Patrick McHardy
2009-09-07 17:30 ` Eric Dumazet
2009-09-07 17:33 ` Patrick McHardy
2009-09-07 17:38 ` Eric Dumazet
2009-09-07 17:46 ` Patrick McHardy
2009-09-08 9:31 ` David Miller
2009-09-08 15:53 ` Patrick McHardy
2009-09-05 7:27 ` David Miller
2009-09-05 17:02 ` Patrick McHardy
2009-09-06 9:01 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4AA50A49.7010905@trash.net \
--to=kaber@trash$(echo .)net \
--cc=jarkao2@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox