Date:	Fri, 2 Dec 2011 09:18:05 +0900
From:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Jeremy Fitzhardinge <jeremy.fitzhardinge@...rix.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Jason Baron <jbaron@...hat.com>
Subject: Re: [PATCH] jump_label: jump_label for boot options.

On Thu, 01 Dec 2011 09:48:47 -0500
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Thu, 2011-12-01 at 11:53 +0900, KAMEZAWA Hiroyuki wrote:
> > I tried to use jump_label for handling memcg's boot options which sets
> > global variable true/false and never changes after boot. And found jump_table
> > is larger than expected. This patch is a trial to allow to place jump_table
> > in .init section. How do you think ?
> > 
> > This patch is based on linux-next.
> > 
> > ==
> > >From ed8996892c21d4fec642e4fc80bd3ebdd8a48836 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> > Date: Thu, 1 Dec 2011 11:08:23 +0900
> > Subject: [PATCH] jump_label for boot options.
> > 
> > Some boot options exists for enable/disable a functionality which cannot be modified
> > after boot. Using jump_label for such behavior seems atractive but if caller
> > of statch_branch() is too much, jump_table can be unexpectedly big.
> 
> s/statch/static/
> 
will fix.

> > 
> > This patch adds static_branch_init_once(), which places its jump_table
> > in init section and never be updated after boot.
> 
> I don't like the name static_branch_init_once(). Although I suck at
> picking names myself :-p  Maybe just remove the 'init'.
> static_branch_once(), or remove the 'once'. static_branch_init()
> 

ok.

> Your name may be the best, but I'm hoping another name will come up.
> static_branch_init_once() just doesn't seem right.
> 
I don't think my sense of naming is good.

> 
> > For MODULES, using usual static_branch().
> 
> Why not for modules? It can be forced at module load time, and then
> removed.
> 

It can be. I just don't investigate how module's init section is handled.

<snip>


> > +/*
> > + * Use this when you call static_branch() in __init function or
> > + * jump_label is only modified by initcalls. jump_label information
> > + * will be discarded after boot. But please be careful. jump_label_key
> > + * definition itself should *not* be in __init section because a MODULE
> > + * may call static_branch_init_once().
> > + *
> > + * Useful for changing behavior by boot option.
> > + */
> > +#ifndef MODULE
> > +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> > +{
> > +	return arch_static_branch_init_once(key);
> > +}
> > +#else
> > +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> > +{
> > +	return static_branch(key);
> > +}
> > +#endif
> 
> I still think modules should be able to do this, and then just remove it
> later.
> 

Hm, ok, I'll look into and prepare an another patch.


> > +
> >  extern struct jump_entry __start___jump_table[];
> >  extern struct jump_entry __stop___jump_table[];
> > +extern struct jump_entry __start___jump_table_at_init[];
> > +extern struct jump_entry __stop___jump_table_at_init[];
> >  
> >  extern void jump_label_init(void);
> >  extern void jump_label_lock(void);
> > @@ -54,6 +77,12 @@ extern void jump_label_dec(struct jump_label_key *key);
> >  extern bool jump_label_enabled(struct jump_label_key *key);
> >  extern void jump_label_apply_nops(struct module *mod);
> >  
> > +/*
> > + * For jump_label in init section.
> > + * This will call jump_label_inc() for usual section, too.
> > + */
> > +extern void jump_label_inc_once(struct jump_label_key *key);
> > +
> >  #else  /* !HAVE_JUMP_LABEL */
> >  
> >  #include <linux/atomic.h>
> > @@ -75,6 +104,11 @@ static __always_inline bool static_branch(struct jump_label_key *key)
> >  	return false;
> >  }
> >  
> > +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> > +{
> > +	return static_branch(key);
> > +}
> > +
> >  static inline void jump_label_inc(struct jump_label_key *key)
> >  {
> >  	atomic_inc(&key->enabled);
> > @@ -102,6 +136,11 @@ static inline int jump_label_apply_nops(struct module *mod)
> >  {
> >  	return 0;
> >  }
> > +
> > +static inline void jump_label_inc_once(struct jump_label_key *key)
> > +{
> > +	jump_label_inc(key);
> > +}
> >  #endif	/* HAVE_JUMP_LABEL */
> >  
> >  #endif	/* _LINUX_JUMP_LABEL_H */
> > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > index 66ff710..f0e8231 100644
> > --- a/kernel/jump_label.c
> > +++ b/kernel/jump_label.c
> > @@ -134,15 +134,11 @@ static void __jump_label_update(struct jump_label_key *key,
> >  	}
> >  }
> >  
> > -void __init jump_label_init(void)
> > +void __init jump_label_transform_all(struct jump_entry *iter_start,
> > +				     struct jump_entry *iter_stop)
> >  {
> > -	struct jump_entry *iter_start = __start___jump_table;
> > -	struct jump_entry *iter_stop = __stop___jump_table;
> > -	struct jump_label_key *key = NULL;
> >  	struct jump_entry *iter;
> > -
> > -	jump_label_lock();
> > -	jump_label_sort_entries(iter_start, iter_stop);
> > +	struct jump_label_key *key = NULL;
> >  
> >  	for (iter = iter_start; iter < iter_stop; iter++) {
> >  		struct jump_label_key *iterk;
> > @@ -159,6 +155,24 @@ void __init jump_label_init(void)
> >  		key->next = NULL;
> >  #endif
> >  	}
> > +
> > +}
> > +
> > +
> > +void __init jump_label_init(void)
> > +{
> > +	struct jump_entry *iter_start = __start___jump_table;
> > +	struct jump_entry *iter_stop = __stop___jump_table;
> > +
> > +
> > +	jump_label_lock();
> > +	jump_label_sort_entries(iter_start, iter_stop);
> > +	jump_label_transform_all(iter_start, iter_stop);
> 
> Nit, I'd add a space here.
> 

ok. I'll fix.

Thanks,
-Kame


--
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