[PATCH] pkt_sched: Fix tx queue selection in tc_modify_qdisc

[PATCH] pkt_sched: Fix tx queue selection in tc_modify_qdisc

[Jarek Poplawski]

After the recent mq change there is the new select_queue qdisc class
method used in tc_modify_qdisc, but it works OK only for direct child
qdiscs of mq qdisc. Grandchildren always get the first tx queue, which
would give wrong qdisc_root etc. results (e.g. for sch_htb as child of
sch_prio). This patch fixes it by using parent's dev_queue for such
grandchildren qdiscs. The select_queue method is replaced BTW with the
static qdisc_select_tx_queue function (it's used only in one place).

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 include/net/sch_generic.h |    1 -
 net/sched/sch_api.c       |   29 +++++++++++++++++++++--------
 net/sched/sch_mq.c        |   10 ----------
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 88eb9de..865120c 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -81,7 +81,6 @@ struct Qdisc
 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);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 3af1061..223a6bc 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -990,6 +990,24 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
 	return 0;
 }
 
+static struct netdev_queue *qdisc_select_tx_queue(struct net_device *dev,
+						  struct Qdisc *p, u32 clid)
+{
+	unsigned long ntx;
+
+	if (!p)
+		return netdev_get_tx_queue(dev, 0);
+
+	if (!(p->flags & TCQ_F_MQROOT))
+		return p->dev_queue;
+
+	ntx = TC_H_MIN(clid) - 1;
+	if (ntx >= dev->num_tx_queues)
+		ntx = 0;
+
+	return netdev_get_tx_queue(dev, ntx);
+}
+
 /*
    Create/change qdisc.
  */
@@ -1110,16 +1128,11 @@ create_n_graft:
 		q = qdisc_create(dev, &dev->rx_queue, p,
 				 tcm->tcm_parent, tcm->tcm_parent,
 				 tca, &err);
-	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);
-
-		q = qdisc_create(dev, netdev_get_tx_queue(dev, ntx), p,
+	else
+		q = qdisc_create(dev, qdisc_select_tx_queue(dev, p, clid), p,
 				 tcm->tcm_parent, tcm->tcm_handle,
 				 tca, &err);
-	}
+
 	if (q == NULL) {
 		if (err == -EAGAIN)
 			goto replay;
diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index dd5ee02..4ad949b 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -125,15 +125,6 @@ static struct netdev_queue *mq_queue_get(struct Qdisc *sch, unsigned long cl)
 	return netdev_get_tx_queue(dev, ntx);
 }
 
-static unsigned int mq_select_queue(struct Qdisc *sch, struct tcmsg *tcm)
-{
-	unsigned int ntx = TC_H_MIN(tcm->tcm_parent);
-
-	if (!mq_queue_get(sch, ntx))
-		return 0;
-	return ntx - 1;
-}
-
 static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
 		    struct Qdisc **old)
 {
@@ -213,7 +204,6 @@ static void mq_walk(struct Qdisc *sch, struct qdisc_walker *arg)
 }
 
 static const struct Qdisc_class_ops mq_class_ops = {
-	.select_queue	= mq_select_queue,
 	.graft		= mq_graft,
 	.leaf		= mq_leaf,
 	.get		= mq_get,

Re: [PATCH] pkt_sched: Fix tx queue selection in tc_modify_qdisc

[Patrick McHardy]

Jarek Poplawski wrote:
> After the recent mq change there is the new select_queue qdisc class
> method used in tc_modify_qdisc, but it works OK only for direct child
> qdiscs of mq qdisc. Grandchildren always get the first tx queue, which
> would give wrong qdisc_root etc. results (e.g. for sch_htb as child of
> sch_prio). This patch fixes it by using parent's dev_queue for such
> grandchildren qdiscs. The select_queue method is replaced BTW with the
> static qdisc_select_tx_queue function (it's used only in one place).

Thanks, this looks correct. My assumption was that we shouldn't
be using the locks of grandchildren anyways, but we do need the
proper root lock for estimators.

> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 88eb9de..865120c 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -81,7 +81,6 @@ struct Qdisc
>  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);
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 3af1061..223a6bc 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -990,6 +990,24 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
>  	return 0;
>  }
>  
> +static struct netdev_queue *qdisc_select_tx_queue(struct net_device *dev,
> +						  struct Qdisc *p, u32 clid)
> +{
> +	unsigned long ntx;
> +
> +	if (!p)
> +		return netdev_get_tx_queue(dev, 0);
> +
> +	if (!(p->flags & TCQ_F_MQROOT))
> +		return p->dev_queue;
> +
> +	ntx = TC_H_MIN(clid) - 1;

I didn't want to expose the numbering scheme used by sch_mq internally,
but fine, I see you really don't like the callback :)

Re: [PATCH] pkt_sched: Fix tx queue selection in tc_modify_qdisc

[Jarek Poplawski]

On Mon, Sep 14, 2009 at 08:05:45PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> > After the recent mq change there is the new select_queue qdisc class
> > method used in tc_modify_qdisc, but it works OK only for direct child
> > qdiscs of mq qdisc. Grandchildren always get the first tx queue, which
> > would give wrong qdisc_root etc. results (e.g. for sch_htb as child of
> > sch_prio). This patch fixes it by using parent's dev_queue for such
> > grandchildren qdiscs. The select_queue method is replaced BTW with the
> > static qdisc_select_tx_queue function (it's used only in one place).
> 
> Thanks, this looks correct. My assumption was that we shouldn't
> be using the locks of grandchildren anyways, but we do need the
> proper root lock for estimators.

Actually, in the above example I was mainly concerned with a watchdog
parameter. But of course there should more (etc.).

> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index 88eb9de..865120c 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -81,7 +81,6 @@ struct Qdisc
> >  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);
> > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > index 3af1061..223a6bc 100644
> > --- a/net/sched/sch_api.c
> > +++ b/net/sched/sch_api.c
> > @@ -990,6 +990,24 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
> >  	return 0;
> >  }
> >  
> > +static struct netdev_queue *qdisc_select_tx_queue(struct net_device *dev,
> > +						  struct Qdisc *p, u32 clid)
> > +{
> > +	unsigned long ntx;
> > +
> > +	if (!p)
> > +		return netdev_get_tx_queue(dev, 0);
> > +
> > +	if (!(p->flags & TCQ_F_MQROOT))
> > +		return p->dev_queue;
> > +
> > +	ntx = TC_H_MIN(clid) - 1;
> 
> I didn't want to expose the numbering scheme used by sch_mq internally,
> but fine, I see you really don't like the callback :)

I only don't like the callback just for one exceptional qdisc. On the
other hand it would look more sensible to me if implemented at least
by all classful qdiscs to return parent's dev_queue always; so I could
re-do it like this, or simply mix this fix with the current
implementation, no problem (I don't "don't like it" too much).

Jarek P.

[PATCH v2] pkt_sched: Fix tx queue selection in tc_modify_qdisc

[Jarek Poplawski]

On Mon, Sep 14, 2009 at 08:05:45PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> > After the recent mq change there is the new select_queue qdisc class
> > method used in tc_modify_qdisc, but it works OK only for direct child
> > qdiscs of mq qdisc. Grandchildren always get the first tx queue, which
> > would give wrong qdisc_root etc. results (e.g. for sch_htb as child of
> > sch_prio). This patch fixes it by using parent's dev_queue for such
> > grandchildren qdiscs. The select_queue method is replaced BTW with the
> > static qdisc_select_tx_queue function (it's used only in one place).
> 
> Thanks, this looks correct. My assumption was that we shouldn't
> be using the locks of grandchildren anyways, but we do need the
> proper root lock for estimators.
...
> I didn't want to expose the numbering scheme used by sch_mq internally,
> but fine, I see you really don't like the callback :)

So here is an alternative version with the callback saved (the return
type is changed) for consideration.

Thanks,
Jarek P.
-----------------------> (take 2)
pkt_sched: Fix tx queue selection in tc_modify_qdisc

After the recent mq change there is the new select_queue qdisc class
method used in tc_modify_qdisc, but it works OK only for direct child
qdiscs of mq qdisc. Grandchildren always get the first tx queue, which
would give wrong qdisc_root etc. results (e.g. for sch_htb as child of
sch_prio). This patch fixes it by using parent's dev_queue for such
grandchildren qdiscs. The select_queue method's return type is changed
BTW.

With feedback from: Patrick McHardy <kaber@trash.net>

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 include/net/sch_generic.h |    2 +-
 net/sched/sch_api.c       |   10 +++++++---
 net/sched/sch_mq.c        |   13 +++++++++----
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 88eb9de..c33180d 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -81,7 +81,7 @@ struct Qdisc
 struct Qdisc_class_ops
 {
 	/* Child qdisc manipulation */
-	unsigned int		(*select_queue)(struct Qdisc *, struct tcmsg *);
+	struct netdev_queue *	(*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);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 3af1061..a1184b2 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1111,12 +1111,16 @@ create_n_graft:
 				 tcm->tcm_parent, tcm->tcm_parent,
 				 tca, &err);
 	else {
-		unsigned int ntx = 0;
+		struct netdev_queue *dev_queue;
 
 		if (p && p->ops->cl_ops && p->ops->cl_ops->select_queue)
-			ntx = p->ops->cl_ops->select_queue(p, tcm);
+			dev_queue = p->ops->cl_ops->select_queue(p, tcm);
+		else if (p)
+			dev_queue = p->dev_queue;
+		else
+			dev_queue = netdev_get_tx_queue(dev, 0);
 
-		q = qdisc_create(dev, netdev_get_tx_queue(dev, ntx), p,
+		q = qdisc_create(dev, dev_queue, p,
 				 tcm->tcm_parent, tcm->tcm_handle,
 				 tca, &err);
 	}
diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index dd5ee02..600c501 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -125,13 +125,18 @@ static struct netdev_queue *mq_queue_get(struct Qdisc *sch, unsigned long cl)
 	return netdev_get_tx_queue(dev, ntx);
 }
 
-static unsigned int mq_select_queue(struct Qdisc *sch, struct tcmsg *tcm)
+static struct netdev_queue *mq_select_queue(struct Qdisc *sch,
+					    struct tcmsg *tcm)
 {
 	unsigned int ntx = TC_H_MIN(tcm->tcm_parent);
+	struct netdev_queue *dev_queue = mq_queue_get(sch, ntx);
 
-	if (!mq_queue_get(sch, ntx))
-		return 0;
-	return ntx - 1;
+	if (!dev_queue) {
+		struct net_device *dev = qdisc_dev(sch);
+
+		return netdev_get_tx_queue(dev, 0);
+	}
+	return dev_queue;
 }
 
 static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,

Re: [PATCH v2] pkt_sched: Fix tx queue selection in tc_modify_qdisc

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 15 Sep 2009 00:50:22 +0200

> -----------------------> (take 2)
> pkt_sched: Fix tx queue selection in tc_modify_qdisc
> 
> After the recent mq change there is the new select_queue qdisc class
> method used in tc_modify_qdisc, but it works OK only for direct child
> qdiscs of mq qdisc. Grandchildren always get the first tx queue, which
> would give wrong qdisc_root etc. results (e.g. for sch_htb as child of
> sch_prio). This patch fixes it by using parent's dev_queue for such
> grandchildren qdiscs. The select_queue method's return type is changed
> BTW.
> 
> With feedback from: Patrick McHardy <kaber@trash.net>
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied, thanks Jarek.