On Wed, 2005-28-12 at 18:19 +0100, Patrick McHardy wrote:
> While I have nothing against the idea, the patch needs a lot
> of cleanup. You seem to add almost as much ifdefed code to
> dummy as it currently has, which makes me wonder if a completely
> new device wouldn't be much cleaner.
>
Its one of those 50/50 decisions - I could create a new device,
what would you think for a name?
Note that: The existence of dummy is for very simple reasons - to
blackhole packets. It has all the infrastructure code needed; I just
added one optional use. Dummy hasnt been touched in > 10 years, someone
needed to do it ;->
> jamal wrote:
> > + /* mostly debug stats leave in for now */
> > + unsigned long stat_te; /* tasklet entered */
> > + unsigned long stat_tqr; /* transmit queue refill attempt */
> > + unsigned long stat_rqe; /* receive queue entered */
[..]
> I would prefer more descriptive names, like tasklet_entered,
> txqueue_refill_attempt, ...
>
I had the names as long as you describe them and eventually abbreviated
them because it was getting annoying to space them. Arent the comments
sufficient? note i eventually would like to kill these stats in any
case. I have them in there as performance measurement tool.
> I also don't see a way to actually read these stats, except by
> enabling debugging.
>
Yes, thats is the intent.
I am eventually going to remove these stats future.
> > +#ifdef CONFIG_NET_CLS_ACT
> > +static void ri_tasklet(unsigned long dev);
> > +#endif
>
> Please avoid the excessive use of ifdefs. Unused forward-declarations
> don't hurt. It seems to be unnecessary anyway.
>
Ok, I will remove ifdefs where it is obvious they can be removed.
Note: I want to maintain the dummy device as is when noone is
redirecting packets to from the classifier action.
>
> > +
> > + int cpu = smp_processor_id();
> > +
> > + dev->trans_start = jiffies;
> > + printk("%s: BUG tx timeout on CPU %d\n",dev->name,cpu);
> > + if (spin_is_locked((&dev->xmit_lock)))
> > + printk("xmit lock grabbed already\n");
> > + if (spin_is_locked((&dev->queue_lock)))
> > + printk("queue lock grabbed already\n");
> > +}
>
> The watchdog always takes xmit_lock,
Good point.
> and these "... lock grabbed
> already" printks look totally useless.
>
If the watchdog is entered, there must be a bug somewhere. If there is
useful information to printk, it is a good idea to do so. I am going
to leave the queue lock printk in.
> > + dp->stat_tqr +=1;
> > + if (spin_trylock(&dv->xmit_lock)) {
> > + dp->stat_rqe +=1;
> > + while (NULL != (skb = skb_dequeue(&dp->rq))) {
> > + skb_queue_tail(&dp->tq, skb);
>
> Looks like you want skb_move or something like that. Why do
> you need this shoving skbs between queues anyway? You pull
> them out of the queue again right below.
packets are moved from receive -> transmit queues only when the transmit
is empty. The tx lock is only held at that point. This is
consistent with current way drivers do things and provides some
good performance numbers.
> > + } else {
> > + /* if netfilt is compiled in and packet is
> > + tagged, we could reinject the packet back
> > + this would make it do remaining 10%
> > + of what current IMQ does
> > + if someone really really insists then
> > + this is the spot .. jhs */
> > + dev_kfree_skb(skb);
> > + stats->tx_dropped++;
>
> Packets that are queued from netfilter must not be freed other
> than by passing them to nf_reinject with a verdict of drop.
> It doesn't look as if you were using netfilter at all, but
> at least that comment is wrong.
>
That just points to a spot where the change could be made,
not necessarily how it is done;-> If ever netfilter was added, it would
have to be done properly. Does this sound reasonable?
I could add more comments to the effect which you specified.
> > +dropped:
> > + dev_kfree_skb(skb);
> > + stats->rx_dropped++;
> > + return ret;
> > + } else {
> > + if (skb->input_dev)
> > + skb->dev = skb->input_dev;
> > + else
> > + printk("warning!!! no idev %s\n",skb->dev->name);
> > +
> > + skb->input_dev = dev;
>
> Is that still necessary and/or correct? We moved the input_dev
> assignment to netif_receive_skb().
>
The check inside the else for skb->input_dev is unnecessary (since we
never get in the else without skb->input_dev being set). So i will
remove that.
Again, as i described in emails a while back, on this device, packets
could be going from egress->ingress or egress->ingress.
I will take a look at netif_receive_skb() and see if it is sufficient
while i fix up based on your comments.
Thanks a lot for your feed back Patrick.
cheers,
jamal
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
