Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755524Ab3CUCpI (ORCPT ); Wed, 20 Mar 2013 22:45:08 -0400 Received: from mail.openrapids.net ([64.15.138.104]:52667 "EHLO blackscsi.openrapids.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753447Ab3CUCpG (ORCPT ); Wed, 20 Mar 2013 22:45:06 -0400 Date: Wed, 20 Mar 2013 22:45:02 -0400 From: Mathieu Desnoyers To: Keun-O Park Cc: Steven Rostedt , keun-o.park@windriver.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] tracepoints: prevents null probe from being added Message-ID: <20130321024502.GB3874@Krystal> References: <1363749497-12176-1-git-send-email-kpark3469@gmail.com> <1363800712.6345.17.camel@gandalf.local.home> <20130320180113.GA24537@Krystal> <1363820491.6345.21.camel@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Editor: vi X-Info: http://www.efficios.com User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4470 Lines: 136 * Keun-O Park (kpark3469@gmail.com) wrote: > On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt wrote: > > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote: > >> * Steven Rostedt (rostedt@goodmis.org) wrote: > >> > On Wed, 2013-03-20 at 12:18 +0900, kpark3469@gmail.com wrote: > >> > > From: Sahara > >> > > > >> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe > >> > > function. > >> > > >> > You actually hit this in practice, or is this just something that you > >> > observe from code review? > >> > > >> > > Especially on getting a null probe in remove function, it seems > >> > > to be used to remove all probe functions in the entry. > >> > > >> > Hmm, that actually sounds like a feature. > >> > >> Yep. It's been a long time since I wrote this code, but the removal code > >> seems to use NULL probe pointer to remove all probes for a given > >> tracepoint. > >> > >> I'd be tempted to just validate non-NULL probe within > >> tracepoint_entry_add_probe() and let other sites as is, just in case > >> anyone would be using this feature. > >> > >> I cannot say that I have personally used this "remove all" feature much > >> though. > >> > > > > I agree. I don't see anything wrong in leaving the null probe feature in > > the removal code. But updating the add code looks like a proper change. > > > > -- Steve > > > > > > Hello Steve & Mathieu, > If we want to leave the null probe feature enabled, I think it would > be better modifying the code like the following for code efficiency. > > @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry, > int nr_probes = 0; > struct tracepoint_func *old, *new; > > - WARN_ON(!probe); > + if (WARN_ON(!probe)) > + return ERR_PTR(-EINVAL); > > debug_print_probes(entry); > old = entry->funcs; > @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *ent > > debug_print_probes(entry); > /* (N -> M), (N > 1, M >= 0) probes */ > - for (nr_probes = 0; old[nr_probes].func; nr_probes++) { > - if (!probe || > - (old[nr_probes].func == probe && > - old[nr_probes].data == data)) > - nr_del++; > + if (probe) { > + for (nr_probes = 0; old[nr_probes].func; nr_probes++) { > + if (old[nr_probes].func == probe && > + old[nr_probes].data == data) > + nr_del++; > + } > } > > - if (nr_probes - nr_del == 0) { > + if (!probe || nr_probes - nr_del == 0) { We might want to do: if (probe) { ... } else { nr_del = nr_probes; } if (nr_probes - nr_del == 0) { ... } rather than: if (probe) { ... } if (!probe || nr_probes - nr_del == 0) { ... } Using nr_del makes the code easier to follow IMHO. Thanks, Mathieu > /* N -> 0, (N > 1) */ > entry->funcs = NULL; > entry->refcount = 0; > > Because we know handing over the null probe to > tracepoint_entry_add_probe is not possible, > we don't have to check if the probe is null or not within for loop. If > the probe is null, it's just enough to add !probe in > 'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping > for-loop, falling through for-loop can be prevented when probe is > null. > > @@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry > if (new == NULL) > return ERR_PTR(-ENOMEM); > for (i = 0; old[i].func; i++) > - if (probe && > - (old[i].func != probe || old[i].data != data)) > + if (old[i].func != probe || old[i].data != data) > new[j++] = old[i]; > new[nr_probes - nr_del].func = NULL; > entry->refcount = nr_probes - nr_del; > > We don't have to check the probe here too. We know probe is always true here. > Thanks. > > -- Kpark -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com -- 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/