Re: [RFC] make qdisc_restart more readable

[jamal <hadi@cyberus.ca>]

On Fri, 2007-11-05 at 19:01 +0200, Thomas Graf wrote:
> * jamal <j.hadi123@gmail.com> 2007-05-10 20:13

> >   *	Compute the worst case header length according to the protocols
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index f28bb2d..b821040 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -59,7 +59,79 @@ void qdisc_unlock_tree(struct net_device *dev)
> >  	spin_unlock_bh(&dev->queue_lock);
> >  }
> >  
> > -/*
> > +static inline int qqlen(struct Qdisc *q)
> > +{
> > +	BUG_ON((int) q->q.qlen < 0);
> > +	return q->q.qlen;
> > +}
> 
> Nitpicking but qdisc_qlen() sounds a lot better to me.
> 

I thought nitpicking was my department ;->
qdisc_qlen is more descriptive; i will change it.


> > +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);
> > +
> > +	if (skb->next)
> 
> Oops, missing else here, this will crash.
> 

tabarnaq. I will fix - thanks.

> Seems simpler to just return the skb or NULL instead
> 

sure.

> > +	/* churn baby churn .. */
> > +	ret = dev_hard_start_xmit(skb, dev);
> 
> Check whether queue is stopped got lost here
> 

Also caught by Peter - will fix.




> > +	q = dev->qdisc;
> 
> Maybe comment why q needs to be refreshed so nobody removes
> it by accident.

Makes sense ..

> > +	/* most likely result, packet went ok */
> > +	if (ret == NETDEV_TX_OK)
> > +		return qqlen(q);
> > +	/* only for lockless drivers .. */
> > +	if (ret == NETDEV_TX_LOCKED && lockless) {
> > +		return handle_tx_locked(skb, dev, q);
> > +	}
> 
> Your style of placing parentheses seems to vary randomly :-)

There may have been a debug in there that i removed. Frankly
i dont mind having braces even for a one liner, but i know that is not
popular with many folks.
Any other spot you noticed?

Thanks a lot Thomas.

cheers,
jamal