Re: [RFC PATCH 1/3] static_call: Add static call infrastructure

[Josh Poimboeuf]

On Fri, Nov 09, 2018 at 02:57:46PM -0500, Steven Rostedt wrote:
> On Fri, 9 Nov 2018 13:35:05 -0600
> Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> 
> 
> > > > +#define DECLARE_STATIC_CALL(key, func)					\
> > > > +	extern struct static_call_key key;				\
> > > > +	extern typeof(func) STATIC_CALL_TRAMP(key);			\
> > > > +	/* Preserve the ELF symbol so objtool can access it: */		\
> > > > +	__ADDRESSABLE(key)  
> > > 
> > > Does the __ADDRESSABLE(key) need to be in the DECLARE part?
> > > If so, there needs to be more explanation than just the comment above
> > > it.  
> > 
> > For each call site, objtool creates a struct in .static_call_sites:
> > 
> > 	struct static_call_site {
> > 		s32 addr;
> > 		s32 key;
> > 	};
> > 
> > In order to do that, it needs to create a relocation which references
> > the key symbol.  If the key is defined in another .o file, then the
> > current .o will not have an ELF symbol associated with the key.  The
> > __ADDRESSABLE(key) thing tells GCC to leave the key symbol in the .o
> > file, even though it's not referenced anywhere.  That makes objtool's
> > job easier, so it doesn't have to edit the symbol table.
> > 
> > I could add a comment saying as much, though it's hard to explain it in
> > fewer words than I just did :-)
> 
> Does this have to do with adding the references by relative address?
> 
> In record_mcount, I just picked an existing symbol and referenced that..
> But perhaps this is a cleaner way.

I think recordmcount is different.  It creates references (in
__mcount_loc) to functions which are already in the object file, so they
already have symbols associated with them.

But in this case, when objtool is creating references, the symbol it
needs to reference is outside the .o file, so there's no symbol to
associate it with.

> Adding a more in depth comment wont hurt.
> 
> > 
> > > > +	/*
> > > > +	 * If called before init, leave the call sites unpatched for now.
> > > > +	 * In the meantime they'll continue to call the temporary trampoline.
> > > > +	 */
> > > > +	if (!static_call_initialized)
> > > > +		goto done;
> > > > +
> > > > +	list_for_each_entry(mod, &key->site_mods, list) {  
> > > 
> > > Since I'm expecting a lot of sites, I'm wondering if we should just do
> > > this as an array, like I do with the ftrace call sites.
> > > 
> > > But this can be an enhancement for later. Let's focus on getting this
> > > working first.  
> > 
> > But it's not a static list.  It can grow/shrink as modules are
> > loaded/unload.
> 
> Neither is ftrace :-) What I did was make one array for the core kernel
> code, and an array for each module, and link list those (single link,
> although double link may not be hard either). That will save a lot of
> memory than having each instance have a link pointer, as it only grows
> or shrinks in chunks.

That sounds exactly like what I did :-)

The site_mods list is a list of static_call_mods.  Each static_call_mod
has a pointer to an array (which is that module/vmlinux's
.static_call_site section).

> > > So what's the reason for skipping init calls?  
> > 
> > This is the runtime changing code (static_call_update).  Presumably the
> > init sections no longer exist and we shouldn't write to any (former)
> > call sites there.
> > 
> > That's probably a dangerous assumption though...  If
> > static_call_update() were called early, some init code might not get
> > patched and then call into the wrong function.
> > 
> > I'm thinking we should just disallow static call sites in init sections.
> > I can't think of a good reason why they would be needed in init code.
> > We can WARN when detecting them during boot / module init.
> > 
> 
> What I would do is to allow init (like ftrace now does). I have
> ftrace_free_init_mem() that removes all the mcount references for init
> calls from its list. You could add a static_call_free_init() to
> kernel_init() in init/main.c too.

That makes sense for ftrace, but I don't see much point in allowing it
for static calls.  Maybe we could just add support for it later if it
turns out to be useful.

-- 
Josh