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 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 ?

Mathieu

>
> >
> > 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: Neil Horman <nhorman@tuxdriver.com>
> > CC: Thomas Gleixner <tglx@linutronix.de>
> > ---
> > 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 = skb ? ntohs(skb->protocol) : 0;
> > __entry->location = location;
> > ),
> >
> >
> >

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


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