[Xen-devel] Re: [PATCH RFC V2 3/5] jump_label: if a key has already been initialized, don't nop it out

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

On 10/10/2011 08:36 AM, Jason Baron wrote:
> On Sat, Oct 01, 2011 at 02:55:35PM -0700, Jeremy Fitzhardinge wrote:
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>
>>
>> If a key has been enabled before jump_label_init() is called, don't
>> nop it out.
>>
>> This removes arch_jump_label_text_poke_early() (which can only nop
>> out a site) and uses arch_jump_label_transform() instead.
>>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>
>> ---
>>  include/linux/jump_label.h |    3 ++-
>>  kernel/jump_label.c        |   20 ++++++++------------
>>  2 files changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
>> index 1213e9d..c8fb1b3 100644
>> --- a/include/linux/jump_label.h
>> +++ b/include/linux/jump_label.h
>> @@ -45,7 +45,8 @@ extern void jump_label_lock(void);
>>  extern void jump_label_unlock(void);
>>  extern void arch_jump_label_transform(struct jump_entry *entry,
>>                               enum jump_label_type type);
>> -extern void arch_jump_label_text_poke_early(jump_label_t addr);
>> +extern void arch_jump_label_transform_early(struct jump_entry *entry,
>> +                             enum jump_label_type type);
>>  extern int jump_label_text_reserved(void *start, void *end);
>>  extern void jump_label_inc(struct jump_label_key *key);
>>  extern void jump_label_dec(struct jump_label_key *key);
>> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
>> index a8ce450..059202d5 100644
>> --- a/kernel/jump_label.c
>> +++ b/kernel/jump_label.c
>> @@ -121,13 +121,6 @@ static void __jump_label_update(struct jump_label_key 
>> *key,
>>      }
>>  }
>>  
>> -/*
>> - * Not all archs need this.
>> - */
>> -void __weak arch_jump_label_text_poke_early(jump_label_t addr)
>> -{
>> -}
>> -
>>  static __init int jump_label_init(void)
>>  {
>>      struct jump_entry *iter_start = __start___jump_table;
>> @@ -139,12 +132,15 @@ static __init int jump_label_init(void)
>>      jump_label_sort_entries(iter_start, iter_stop);
>>  
>>      for (iter = iter_start; iter < iter_stop; iter++) {
>> -            arch_jump_label_text_poke_early(iter->code);
>> -            if (iter->key == (jump_label_t)(unsigned long)key)
>> +            struct jump_label_key *iterk;
>> +
>> +            iterk = (struct jump_label_key *)(unsigned long)iter->key;
>> +            arch_jump_label_transform(iter, jump_label_enabled(iterk) ?
>> +                                      JUMP_LABEL_ENABLE : 
>> JUMP_LABEL_DISABLE);
>> +            if (iterk == key)
>>                      continue;
>>  
>> -            key = (struct jump_label_key *)(unsigned long)iter->key;
>> -            atomic_set(&key->enabled, 0);
>> +            key = iterk;
>>              key->entries = iter;
>>  #ifdef CONFIG_MODULES
>>              key->next = NULL;
>> @@ -212,7 +208,7 @@ void jump_label_apply_nops(struct module *mod)
>>              return;
>>  
>>      for (iter = iter_start; iter < iter_stop; iter++)
>> -            arch_jump_label_text_poke_early(iter->code);
>> +            arch_jump_label_transform(iter, JUMP_LABEL_DISABLE);
>>  }
>>  
>>  static int jump_label_add_module(struct module *mod)
>> -- 
>> 1.7.6.2
>>
> Hi,
>
> I just realized that the early call to jump_label_inc(), isn't being
> honored with this patch until later when we invoke jump_label_init().
> That strikes me as being inconsistent. When jump_label_inc() returns we
> should expect the branch to be updated.

Why is that?  It looks to me like it will unconditionally update the
instruction, irrespective of whether _init() has been called?

> Thus, I think what probably want is to add a new 'int jump_label_init'
> flag. If its not set we can call 'jump_label_init()' from
> jump_label_inc()/dec().

Hm.  I worry that it may end up calling jump_label_init() in an
unexpected context, especially since it may well be config-dependent, or
adding a jump_label_inc() later on starts mysteriously failing.

>  And jump_label_init() can avoid initialization
> if its already set.

That doesn't seem worthwhile in itself.  I suspect the number of "early"
jump_label_incs will be very small (or we should look at doing the init
earlier).

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel