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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1178999773.15622.44.camel@localhost>
Date:	Sat, 12 May 2007 15:56:13 -0400
From:	jamal <hadi@...erus.ca>
To:	Patrick McHardy <kaber@...sh.net>
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

On Sat, 2007-12-05 at 19:02 +0200, Patrick McHardy wrote:
> jamal wrote:
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h

> >  #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.
> 

I can make those part of a different namespace; 
put defines in sch_generic sound reasonable?

> > +static inline int handle_dev_cpu_collision(struct net_device *dev)
> > +{
> > +	if (dev->xmit_lock_owner == smp_processor_id()) {
> 
> unlikely would make sense here.
> 

sure.

> > +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.
> 

Good catch: That was a patch from Herbert after Thomas' "q refresh"
patch. So i was just replicating. I will fix it. Note: I think that
change may have made it to -stable?

> > +	else if (skb->next)
> > +		dev->gso_skb = skb;
> 
> The gso_skb cases could probably also be marked unlikely.
> 

will do.

> > +
> > +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()?
> 

dev_tx_islocked()?

> > +	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.

Ok. 

Thanks a lot Patrick.

cheers,
jamal

-
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