Re: [PATCH] tcpdump may trace some outbound packets twice.

[Stephen Hemminger]

On Mon, 15 May 2006 16:11:05 -0700 (PDT)
Ranjit Manomohan <ranjitm@xxxxxxxxxx> wrote:

> On Mon, 15 May 2006, David S. Miller wrote:
> 
> > From: Ranjit Manomohan <ranjitm@xxxxxxxxxx>
> > Date: Mon, 15 May 2006 14:19:06 -0700 (PDT)
> > 
> > > Heres a new version which does a copy instead of the clone to avoid
> > > the double cloning issue.
> > 
> > I still very much dislike this patch because it is creating
> > 1 more clone per packet than is actually necessary and that
> > is very expensive.
> > 
> > dev_queue_xmit_nit() is going to clone whatever SKB you send into
> > there, so better to just bump the reference count (with skb_get())
> > instead of cloning or copying.
> > 
> 
> I was a bit apprehensive about just incrementing the refcnt but that works 
> too. Attached is the modified version.
> 
> -Thanks,
> Ranjit
> 
> --- linux-2.6/net/sched/sch_generic.c	2006-05-10 12:34:52.000000000 -0700
> +++ linux/net/sched/sch_generic.c	2006-05-15 15:48:03.000000000 -0700
> @@ -136,8 +136,12 @@
>  
>  			if (!netif_queue_stopped(dev)) {
>  				int ret;
> +				struct sk_buff *skbc = NULL;
> +				/* Increment the reference count on the skb so
> +				 * that we can use it after a successful xmit.
> +				 */
>  				if (netdev_nit)
> -					dev_queue_xmit_nit(skb, dev);
> +					skbc = skb_get(skb);

				skbc = netdev_nit ? skb_get(skb) : NULL;
>  
>  				ret = dev->hard_start_xmit(skb, dev);
>  				if (ret == NETDEV_TX_OK) { 
> @@ -145,9 +149,20 @@
>  						dev->xmit_lock_owner = -1;
>  						spin_unlock(&dev->xmit_lock);
>  					}
> +					if (skbc) {
> +						/* transmit succeeded, 
> +						 * trace the buffer. */
> +						dev_queue_xmit_nit(skbc,dev);
> +						kfree_skb(skbc);
> +					}
>  					spin_lock(&dev->queue_lock);
>  					return -1;
>  				}
> +
> +				/* Call free in case we incremented refcnt */
> +				if (skbc)
> +					kfree_skb(skbc);

kfree_skb(NULL) is legal so the conditional here is unneeded.

But the increased calls to kfree_skb(NULL) would probably bring the
"unlikely()" hordes descending on kfree_skb, so maybe:

				if (unlikely(netdev_nit))
					kfree_skb(skbc);


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/