lkml.org 
[lkml]   [2011]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC patch 2/5] trace event skb fix unassigned field
    * Frederic Weisbecker (fweisbec@gmail.com) wrote:
    > On Tue, Jan 04, 2011 at 07:40:38PM -0500, Mathieu Desnoyers wrote:
    > > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
    > > > On Tue, Jan 04, 2011 at 06:46:06PM -0500, nhorman@tuxdriver.com wrote:
    > > > > Acked- by: Neil Horman <nhorman@tuxdriver.com>
    > > > >
    > > > >
    > > > > Sent from my Verizon Wireless Phone
    > > > >
    > > > > ----- Reply message -----
    > > > > From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
    > > > > Date: Tue, Jan 4, 2011 6:16 pm
    > > > > Subject: [RFC patch 2/5] trace event skb fix unassigned field
    > > > > To: "LKML" <linux-kernel@vger.kernel.org>
    > > > > Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, "Steven Rostedt" <rostedt@goodmis.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Ingo Molnar" <mingo@elte.hu>, "Neil Horman" <nhorman@tuxdriver.com>, "Thomas Gleixner" <tglx@linutronix.de>
    > > > >
    > > > >
    > > > > The field "protocol" in event kfree_skb is left unassigned if skb is NULL,
    > > > > leaving its trace output as garbage. Assign the value to 0 when skb is NULL
    > > > > instead.
    > > >
    > > > Hm, if the skb is already null, we probably shouldn't send any trace.
    > > >
    > > > What about using TP_CONDITION() ?
    > >
    > > Hrm, let's see. It's been introduced by commit
    > > 5cb3d1d9d34ac04bcaa2034139345b2a5fea54c1
    > > by Zhaolei.
    > >
    > > Event at the time of that commit, the only caller looked like:
    > >
    > > void kfree_skb(struct sk_buff *skb)
    > > {
    > > if (unlikely(!skb))
    > > return;
    > > if (likely(atomic_read(&skb->users) == 1))
    > > smp_rmb();
    > > else if (likely(!atomic_dec_and_test(&skb->users)))
    > > return;
    > > trace_kfree_skb(skb, __builtin_return_address(0));
    > > __kfree_skb(skb);
    > > }
    > > EXPORT_SYMBOL(kfree_skb);
    > >
    > > So it already checks for a null pointer before calling the tracepoint. This
    > > leads me to wonder why why this check was added in the first place ?
    >
    > Likely for no strong reasons :)
    >
    > So I guess we can remove the check from the tracepoint?

    Yep, leading to this patch instead:

    trace event skb remove duplicate null-pointer check

    The check for NULL skb in the kfree_skb trace event is a duplicate from the
    check already done in its only caller, kfree_skb(). Remove this duplicate check.

    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    CC: Steven Rostedt <rostedt@goodmis.org>
    CC: Frederic Weisbecker <fweisbec@gmail.com>
    CC: Ingo Molnar <mingo@elte.hu>
    CC: Thomas Gleixner <tglx@linutronix.de>
    CC: Neil Horman <nhorman@tuxdriver.com>
    ---
    include/trace/events/skb.h | 4 +---
    1 file changed, 1 insertion(+), 3 deletions(-)

    Index: linux-2.6-lttng/include/trace/events/skb.h
    ===================================================================
    --- linux-2.6-lttng.orig/include/trace/events/skb.h
    +++ linux-2.6-lttng/include/trace/events/skb.h
    @@ -25,9 +25,7 @@ TRACE_EVENT(kfree_skb,

    TP_fast_assign(
    __entry->skbaddr = skb;
    - if (skb) {
    - __entry->protocol = ntohs(skb->protocol);
    - }
    + __entry->protocol = ntohs(skb->protocol);
    __entry->location = location;
    ),


    --
    Mathieu Desnoyers
    Operating System Efficiency R&D Consultant
    EfficiOS Inc.
    http://www.efficios.com


    \
     
     \ /
      Last update: 2011-01-05 02:23    [W:3.099 / U:0.632 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site