lkml.org 
[lkml]   [2006]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] tcpdump may trace some outbound packets twice.
    On Mon, 15 May 2006 16:11:05 -0700 (PDT)
    Ranjit Manomohan <ranjitm@google.com> wrote:

    > On Mon, 15 May 2006, David S. Miller wrote:
    >
    > > From: Ranjit Manomohan <ranjitm@google.com>
    > > 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-05-16 01:43    [W:0.024 / U:29.832 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site