| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <48FC40B3.8060308@cn.fujitsu.com>
Date: Mon, 20 Oct 2008 16:26:27 +0800
From: Lai Jiangshan <laijs@...fujitsu.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
CC: Ingo Molnar <mingo@...e.hu>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH tip/tracing/markers] markers: simplify marker_set_format()
Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs@...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.
this fix did help for readability and made code cleanly.
but it's still 1 allocation per marker_entry when format != NULL.
the previous benefit is not lost.
Thanks, Lai
>
> Thanks !
>
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
>
>> Signed-off-by: Lai Jiangshan <laijs@...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;
>> }
>>
>>
>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists