netdev - Re: take 3 [RFC] make qdisc

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]

Date:	Sat, 12 May 2007 19:02:29 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	hadi@...erus.ca
CC:	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>,
	Thomas Graf <tgraf@...g.ch>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: take 3 [RFC] make qdisc_restart more readable

jamal wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index f671cd2..718d6fd 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -83,6 +83,9 @@ struct wireless_dev;
>  #define NETDEV_TX_OK 0		/* driver took care of packet */
>  #define NETDEV_TX_BUSY 1	/* driver tx path was busy*/
>  #define NETDEV_TX_LOCKED -1	/* driver tx lock was already taken */
> +#define NETDEV_TX_DROP -2	/* request caller to drop packet */

This shouldn't be a NETDEV_TX code since its only handled correctly
internally for handle_dev_cpu_collision().

> +#define NETDEV_TX_QUEUE -3	/* request caller to requeue packet */

How will this be used? The driver can simply stop the queue if
it doesn't want to receive more packets.

> +static inline int handle_dev_cpu_collision(struct net_device *dev)
> +{
> +	if (dev->xmit_lock_owner == smp_processor_id()) {

unlikely would make sense here.

> +static inline int
> +handle_dev_requeue(struct sk_buff *skb, struct net_device *dev, struct Qdisc *q)
> +{
> +
> +	if (unlikely(q == &noop_qdisc))
> +		kfree_skb(skb);

Why is this special-casing needed? __qdisc_run already checks for
noop_qdisc and noop has a proper requeue function.

> +	else if (skb->next)
> +		dev->gso_skb = skb;

The gso_skb cases could probably also be marked unlikely.

> +	else
> +		q->ops->requeue(skb, q);
> +	/* XXX: Could netif_schedule fail? Or is that fact we are
> +	 * requeueing imply the hardware path is closed
> +	 * and even if we fail, some interupt will wake us
> +	*/
> +	netif_schedule(dev);
> +	return 0;
> +}
> +
> +static inline struct sk_buff *
> +try_get_tx_pkt(struct net_device *dev, struct Qdisc *q)
> +{
> +	struct sk_buff *skb = dev->gso_skb;
> +
> +	if (skb)
> +		dev->gso_skb = NULL;
> +	else
> +		skb = q->dequeue(q);
> +
> +	return skb;
> +}
> +
> +static inline int
> +handle_tx_locked(struct sk_buff *skb, struct net_device *dev, struct Qdisc *q)

handle_.* sounds a bit like "I couldn't think of a better name" :)
How about dev_tx_locked()?

> +{
> +	int ret =  (dev);
> +
> +	if (ret == NETDEV_TX_DROP) {
> +		kfree_skb(skb);
> +		return qdisc_qlen(q);
> +	}
> +
> +	return handle_dev_requeue(skb, dev, q);
> +}

> +	if (unlikely (ret != NETDEV_TX_BUSY)) {
> +		/* XXX: Do we need a ratelimit? or put a
> +		 * BUG_ON((int) ret != NETDEV_TX_BUSY) ?
> +		 **/
> +		printk("BUG %s code %d qlen %d\n",dev->name, ret, q->q.qlen);

BUG_ON sounds a bit extreme, net_ratelimit() makes sense.
-
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