netdev - Re: [net-next-2.6 PATCH v2 3/3] 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]

Message-ID: <4D1D17C9.3040500@gmail.com>
Date:	Fri, 31 Dec 2010 00:37:45 +0100
From:	Jarek Poplawski <jarkao2@...il.com>
To:	John Fastabend <john.r.fastabend@...el.com>
CC:	davem@...emloft.net, netdev@...r.kernel.org, hadi@...erus.ca,
	shemminger@...tta.com, tgraf@...radead.org, eric.dumazet@...il.com,
	bhutchings@...arflare.com, nhorman@...driver.com
Subject: Re: [net-next-2.6 PATCH v2 3/3] net_sched: implement a root container
 qdisc sch_mclass

John Fastabend wrote:
> This implements a mclass 'multi-class' queueing discipline that by
> default creates multiple mq qdisc's one for each traffic class. Each
> mq qdisc then owns a range of queues per the netdev_tc_txq mappings.

Is it really necessary to add one more abstraction layer for this,
probably not most often used (or even asked by users), functionality?
Why mclass can't simply do these few things more instead of attaching
(and changing) mq?

...
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 0af57eb..723ee52 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -50,6 +50,7 @@ struct Qdisc {
>  #define TCQ_F_INGRESS		4
>  #define TCQ_F_CAN_BYPASS	8
>  #define TCQ_F_MQROOT		16
> +#define TCQ_F_MQSAFE		32

If every other qdisc added a flag for qdiscs it likes...

> @@ -709,7 +709,13 @@ static void attach_default_qdiscs(struct net_device *dev)
>  		dev->qdisc = txq->qdisc_sleeping;
>  		atomic_inc(&dev->qdisc->refcnt);
>  	} else {
> -		qdisc = qdisc_create_dflt(txq, &mq_qdisc_ops, TC_H_ROOT);
> +		if (dev->num_tc)

Actually, where this num_tc is expected to be set? I can see it inside
mclass only, with unsetting on destruction, but probably I miss something.

> +			qdisc = qdisc_create_dflt(txq, &mclass_qdisc_ops,
> +						  TC_H_ROOT);
> +		else
> +			qdisc = qdisc_create_dflt(txq, &mq_qdisc_ops,
> +						  TC_H_ROOT);
> +
> +static int mclass_init(struct Qdisc *sch, struct nlattr *opt)
> +{
> +	struct net_device *dev = qdisc_dev(sch);
> +	struct mclass_sched *priv = qdisc_priv(sch);
> +	struct netdev_queue *dev_queue;
> +	struct Qdisc *qdisc;
> +	int i, err = -EOPNOTSUPP;
> +	struct tc_mclass_qopt *qopt = NULL;
> +
> +	/* Unwind attributes on failure */
> +	u8 unwnd_tc = dev->num_tc;
> +	u8 unwnd_map[16];

[TC_MAX_QUEUE] ?

> +	struct netdev_tc_txq unwnd_txq[16];
> +
> +	if (sch->parent != TC_H_ROOT)
> +		return -EOPNOTSUPP;
> +
> +	if (!netif_is_multiqueue(dev))
> +		return -EOPNOTSUPP;
> +
> +	if (nla_len(opt) < sizeof(*qopt))
> +		return -EINVAL;
> +	qopt = nla_data(opt);
> +
> +	memcpy(unwnd_map, dev->prio_tc_map, sizeof(unwnd_map));
> +	memcpy(unwnd_txq, dev->tc_to_txq, sizeof(unwnd_txq));
> +
> +	/* If the mclass options indicate that hardware should own
> +	 * the queue mapping then run ndo_setup_tc if this can not
> +	 * be done fail immediately.
> +	 */
> +	if (qopt->hw && dev->netdev_ops->ndo_setup_tc) {
> +		priv->hw_owned = 1;
> +		if (dev->netdev_ops->ndo_setup_tc(dev, qopt->num_tc))
> +			return -EINVAL;
> +	} else if (!qopt->hw) {
> +		if (mclass_parse_opt(dev, qopt))
> +			return -EINVAL;
> +
> +		if (netdev_set_num_tc(dev, qopt->num_tc))
> +			return -ENOMEM;
> +
> +		for (i = 0; i < qopt->num_tc; i++)
> +			netdev_set_tc_queue(dev, i,
> +					    qopt->count[i], qopt->offset[i]);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	/* Always use supplied priority mappings */
> +	for (i = 0; i < 16; i++) {

i < qopt->num_tc ?

> +		if (netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i])) {
> +			err = -EINVAL;
> +			goto tc_err;
> +		}
> +	}
> +
> +	/* pre-allocate qdisc, attachment can't fail */
> +	priv->qdiscs = kcalloc(qopt->num_tc,
> +			       sizeof(priv->qdiscs[0]), GFP_KERNEL);
> +	if (priv->qdiscs == NULL) {
> +		err = -ENOMEM;
> +		goto tc_err;
> +	}
> +
> +	for (i = 0; i < dev->num_tc; i++) {
> +		dev_queue = netdev_get_tx_queue(dev, dev->tc_to_txq[i].offset);

Are these offsets etc. validated?

Jarek P.
--
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