Messages in this thread | | | Date | Wed, 15 Oct 2008 09:44:16 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH tip/tracing/markers] markers: simplify marker_set_format() |
| |
* Lai Jiangshan (laijs@cn.fujitsu.com) wrote: > > current marker_set_format() is complex this patch simplify it, > and decrease the overhead of marker_update_probes(). >
Yep, I think I did put the format string after the name \0, which was rather tricky. I agree that such change makes the code more readable at the expense of doing 2 allocations per marker rather than one, but readability might be a good thing here.
Thanks !
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> > --- > diff --git a/kernel/marker.c b/kernel/marker.c > index e49a9c5..ef43c86 100644 > --- a/kernel/marker.c > +++ b/kernel/marker.c > @@ -65,6 +65,7 @@ struct marker_entry { > void *oldptr; > int rcu_pending; > unsigned char ptype:1; > + unsigned char format_allocated:1; > char name[0]; /* Contains name'\0'format'\0' */ > }; > > @@ -415,6 +416,7 @@ static struct marker_entry *add_marker(const char *name, const char *format) > e->single.probe_private = NULL; > e->multi = NULL; > e->ptype = 0; > + e->format_allocated = 0; > e->refcount = 0; > e->rcu_pending = 0; > hlist_add_head(&e->hlist, head); > @@ -446,6 +448,8 @@ static int remove_marker(const char *name) > if (e->single.func != __mark_empty_function) > return -EBUSY; > hlist_del(&e->hlist); > + if (e->format_allocated) > + kfree(e->format); > /* Make sure the call_rcu has been executed */ > if (e->rcu_pending) > rcu_barrier_sched(); > @@ -456,57 +460,34 @@ static int remove_marker(const char *name) > /* > * Set the mark_entry format to the format found in the element. > */ > -static int marker_set_format(struct marker_entry **entry, const char *format) > +static int marker_set_format(struct marker_entry *entry, const char *format) > { > - struct marker_entry *e; > - size_t name_len = strlen((*entry)->name) + 1; > - size_t format_len = strlen(format) + 1; > - > - > - e = kmalloc(sizeof(struct marker_entry) + name_len + format_len, > - GFP_KERNEL); > - if (!e) > + entry->format = kstrdup(format, GFP_KERNEL); > + if (!entry->format) > return -ENOMEM; > - memcpy(&e->name[0], (*entry)->name, name_len); > - e->format = &e->name[name_len]; > - memcpy(e->format, format, format_len); > - if (strcmp(e->format, MARK_NOARGS) == 0) > - e->call = marker_probe_cb_noarg; > - else > - e->call = marker_probe_cb; > - e->single = (*entry)->single; > - e->multi = (*entry)->multi; > - e->ptype = (*entry)->ptype; > - e->refcount = (*entry)->refcount; > - e->rcu_pending = 0; > - hlist_add_before(&e->hlist, &(*entry)->hlist); > - hlist_del(&(*entry)->hlist); > - /* Make sure the call_rcu has been executed */ > - if ((*entry)->rcu_pending) > - rcu_barrier_sched(); > - kfree(*entry); > - *entry = e; > + entry->format_allocated = 1; > + > trace_mark(core_marker_format, "name %s format %s", > - e->name, e->format); > + entry->name, entry->format); > return 0; > } > > /* > * Sets the probe callback corresponding to one marker. > */ > -static int set_marker(struct marker_entry **entry, struct marker *elem, > +static int set_marker(struct marker_entry *entry, struct marker *elem, > int active) > { > int ret; > - WARN_ON(strcmp((*entry)->name, elem->name) != 0); > + WARN_ON(strcmp(entry->name, elem->name) != 0); > > - if ((*entry)->format) { > - if (strcmp((*entry)->format, elem->format) != 0) { > + if (entry->format) { > + if (strcmp(entry->format, elem->format) != 0) { > printk(KERN_NOTICE > "Format mismatch for probe %s " > "(%s), marker (%s)\n", > - (*entry)->name, > - (*entry)->format, > + entry->name, > + entry->format, > elem->format); > return -EPERM; > } > @@ -522,34 +503,33 @@ static int set_marker(struct marker_entry **entry, struct marker *elem, > * pass from a "safe" callback (with argument) to an "unsafe" > * callback (does not set arguments). > */ > - elem->call = (*entry)->call; > + elem->call = entry->call; > /* > * Sanity check : > * We only update the single probe private data when the ptr is > * set to a _non_ single probe! (0 -> 1 and N -> 1, N != 1) > */ > WARN_ON(elem->single.func != __mark_empty_function > - && elem->single.probe_private > - != (*entry)->single.probe_private && > - !elem->ptype); > - elem->single.probe_private = (*entry)->single.probe_private; > + && elem->single.probe_private != entry->single.probe_private > + && !elem->ptype); > + elem->single.probe_private = entry->single.probe_private; > /* > * Make sure the private data is valid when we update the > * single probe ptr. > */ > smp_wmb(); > - elem->single.func = (*entry)->single.func; > + elem->single.func = entry->single.func; > /* > * We also make sure that the new probe callbacks array is consistent > * before setting a pointer to it. > */ > - rcu_assign_pointer(elem->multi, (*entry)->multi); > + rcu_assign_pointer(elem->multi, entry->multi); > /* > * Update the function or multi probe array pointer before setting the > * ptype. > */ > smp_wmb(); > - elem->ptype = (*entry)->ptype; > + elem->ptype = entry->ptype; > elem->state = active; > > return 0; > @@ -593,8 +573,7 @@ void marker_update_probe_range(struct marker *begin, > for (iter = begin; iter < end; iter++) { > mark_entry = get_marker(iter->name); > if (mark_entry) { > - set_marker(&mark_entry, iter, > - !!mark_entry->refcount); > + set_marker(mark_entry, iter, !!mark_entry->refcount); > /* > * ignore error, continue > */ > @@ -656,7 +635,7 @@ int marker_probe_register(const char *name, const char *format, > ret = PTR_ERR(entry); > } else if (format) { > if (!entry->format) > - ret = marker_set_format(&entry, format); > + ret = marker_set_format(entry, format); > else if (strcmp(entry->format, format)) > ret = -EPERM; > } > > > > >
-- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
| |