Re: take 3 [RFC] make qdisc_restart more readable

[jamal <hadi@cyberus.ca>]

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