netdev - Re: [PATCH] pkt_sched: Fix tx queue selection in tc_modify

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: <4AAE85F9.2060600@trash.net>
Date:	Mon, 14 Sep 2009 20:05:45 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Jarek Poplawski <jarkao2@...il.com>
CC:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] pkt_sched: Fix tx queue selection in tc_modify_qdisc

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 :)
--
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