Prev: MAINTAINERS: Remove section SMX UIO Interface
Next: [PATCH] input: fixup X86_MRST selects
From: Frederic Weisbecker on 9 Jun 2010 18:40
On Wed, Jun 09, 2010 at 05:38:57PM -0400, Jason Baron wrote:
> +static int build_jump_label_hashtable(struct jump_entry *start, struct jump_entry *stop)
> +{
> + struct jump_entry *iter, *iter_begin;
> + struct jump_label_entry *entry;
> + int count;
> +
> + sort_jump_label_entries(start, stop);
> + iter = start;
> + while (iter < stop) {
> + entry = get_jump_label_entry((char *)iter->name);
> + if (!entry) {
> + iter_begin = iter;
> + count = 0;
> + while ((iter < stop) &&
> + (strcmp((char *)iter->name,
> + (char *)iter_begin->name) == 0)) {
> + iter++;
> + count++;
> + }




So, you can have multiple entries with the same name? How can that happen
in fact?




> + entry = add_jump_label_entry((char *)iter_begin->name,
> + count, iter_begin);
> + if (IS_ERR(entry))
> + return PTR_ERR(entry);
> + continue;
> + }
> + WARN(1, KERN_ERR "build_jump_hashtable: unexpected entry!\n");



It seems you are going to endless loop in this fail case.



> + }
> + return 0;
> +}
> +
> +/***
> + * jump_label_update - update jump label text
> + * @name - name of the jump label
> + * @type - enum set to JUMP_LABEL_ENABLE or JUMP_LABEL_DISABLE
> + *
> + * Will enable/disable the jump for jump label @name, depending on the
> + * value of @type.
> + *
> + */
> +
> +void jump_label_update(const char *name, enum jump_label_type type)
> +{
> + struct jump_entry *iter;
> + struct jump_label_entry *entry;
> + struct hlist_node *module_node;
> + struct jump_label_module_entry *e_module;
> + int count;
> +
> + mutex_lock(&jump_label_mutex);
> + entry = get_jump_label_entry(name);
> + if (entry) {
> + count = entry->nr_entries;
> + iter = entry->table;
> + while (count--) {
> + if (kernel_text_address(iter->code))
> + arch_jump_label_transform(iter, type);
> + iter++;
> + }



So, this is going to patch multiple times the same value on the
same address in case you have multiple entries for the same name?

That look weird.

BTW, if you can't find the entry, you should perhaps propagate an error.



> + }
> + mutex_unlock(&jump_label_mutex);
> +}
> +
> +static int init_jump_label(void)



This can be __init.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 9 Jun 2010 18:40
On Wed, Jun 09, 2010 at 05:38:57PM -0400, Jason Baron wrote:
> +#define JUMP_TABLE() \
> + . = ALIGN(64); \
> + VMLINUX_SYMBOL(__start___jump_table) = .; \
> + *(__jump_table) \
> + VMLINUX_SYMBOL(__stop___jump_table) = .; \
> +



Why does it need to be aligned to 64?

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 10 Jun 2010 08:10
On Wed, 2010-06-09 at 17:38 -0400, Jason Baron wrote:
> +static void sort_jump_label_entries(struct jump_entry *start, struct jump_entry *stop)
> +{
> + int swapped = 0;
> + struct jump_entry *iter;
> + struct jump_entry *iter_next;
> +
> + do {
> + swapped = 0;
> + iter = start;
> + iter_next = start;
> + iter_next++;
> + for (; iter_next < stop; iter++, iter_next++) {
> + if (strcmp((char *)iter->name,
> + (char *)iter_next->name) > 0) {
> + swap_jump_label_entries(iter, iter_next);
> + swapped = 1;
> + }
> + }
> + } while (swapped == 1);
> +}

Would lib/sort.c be of any help?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 10 Jun 2010 08:20
On Wed, 2010-06-09 at 17:38 -0400, Jason Baron wrote:

> +#define DEFINE_JUMP_LABEL(name) \
> + const char __jlstrtab_##name[] \
> + __used __attribute__((section("__jump_strings"))) = #name;

> +#define JUMP_LABEL(tag, label, cond) \
> +do { \
> + if (unlikely(cond)) \
> + goto label; \
> +} while (0)

> +#define JUMP_LABEL(tag, label, cond) \
> + do { \
> + extern const char __jlstrtab_##tag[]; \
> + asm goto("1:" \
> + JUMP_LABEL_NOP \
> + ".pushsection __jump_table, \"a\" \n\t"\
> + _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
> + ".popsection \n\t" \
> + : : "i" (__jlstrtab_##tag) : : label);\
> + } while (0)


s/tag/name/ ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jason Baron on 10 Jun 2010 11:50
On Thu, Jun 10, 2010 at 12:35:49AM +0200, Frederic Weisbecker wrote:
> On Wed, Jun 09, 2010 at 05:38:57PM -0400, Jason Baron wrote:
> > +static int build_jump_label_hashtable(struct jump_entry *start, struct jump_entry *stop)
> > +{
> > + struct jump_entry *iter, *iter_begin;
> > + struct jump_label_entry *entry;
> > + int count;
> > +
> > + sort_jump_label_entries(start, stop);
> > + iter = start;
> > + while (iter < stop) {
> > + entry = get_jump_label_entry((char *)iter->name);
> > + if (!entry) {
> > + iter_begin = iter;
> > + count = 0;
> > + while ((iter < stop) &&
> > + (strcmp((char *)iter->name,
> > + (char *)iter_begin->name) == 0)) {
> > + iter++;
> > + count++;
> > + }
>
>
>
>
> So, you can have multiple entries with the same name? How can that happen
> in fact?
>
>

this is the case where a single tracepoint such as kmalloc(), is used in
all over the kernel. So, there is one name or key value associated with
a kmalloc tracepoint. however, we have to patch the jump or nop into a
bunch of places in the kernel text.

>
>
> > + entry = add_jump_label_entry((char *)iter_begin->name,
> > + count, iter_begin);
> > + if (IS_ERR(entry))
> > + return PTR_ERR(entry);
> > + continue;
> > + }
> > + WARN(1, KERN_ERR "build_jump_hashtable: unexpected entry!\n");
>
>
>
> It seems you are going to endless loop in this fail case.
>

agreed. I need to stick that 'WARN' into the else clause of "if
(!entry)" and return an error.

>
>
> > + }
> > + return 0;
> > +}
> > +
> > +/***
> > + * jump_label_update - update jump label text
> > + * @name - name of the jump label
> > + * @type - enum set to JUMP_LABEL_ENABLE or JUMP_LABEL_DISABLE
> > + *
> > + * Will enable/disable the jump for jump label @name, depending on the
> > + * value of @type.
> > + *
> > + */
> > +
> > +void jump_label_update(const char *name, enum jump_label_type type)
> > +{
> > + struct jump_entry *iter;
> > + struct jump_label_entry *entry;
> > + struct hlist_node *module_node;
> > + struct jump_label_module_entry *e_module;
> > + int count;
> > +
> > + mutex_lock(&jump_label_mutex);
> > + entry = get_jump_label_entry(name);
> > + if (entry) {
> > + count = entry->nr_entries;
> > + iter = entry->table;
> > + while (count--) {
> > + if (kernel_text_address(iter->code))
> > + arch_jump_label_transform(iter, type);
> > + iter++;
> > + }
>
>
>
> So, this is going to patch multiple times the same value on the
> same address in case you have multiple entries for the same name?
>
> That look weird.

no. as mentioned before, there are multiple text addresses potentially
associated with a single jump label conditional variable. So we need to
patch all of them.

>
> BTW, if you can't find the entry, you should perhaps propagate an error.
>
>
>
> > + }
> > + mutex_unlock(&jump_label_mutex);
> > +}
> > +
> > +static int init_jump_label(void)
>
>
>
> This can be __init.
>

ok.

thanks for the review.

-Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: MAINTAINERS: Remove section SMX UIO Interface
Next: [PATCH] input: fixup X86_MRST selects