Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290005 > unrolled thread

Re: [PATCHSET v2] ->follow_link() without dropping from RCU mode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHSET v2] ->follow_link() without dropping from RCU mode Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-12 00:20 +0100
    Re: [PATCHSET v2] ->follow_link() without dropping from RCU mode Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-12 03:10 +0100
      Re: [PATCHSET v2] ->follow_link() without dropping from RCU mode Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-12-13 19:50 +0100

#1290005 — Re: [PATCHSET v2] ->follow_link() without dropping from RCU mode

On Fri, Dec 11, 2015 at 08:49:24AM +0100, Rasmus Villemoes wrote:

> union delayed_call_fn {
> 	void (*fn)(void *);
> 	void (*kfree_like)(const void *);
> } __attribute__((__transparent_union__));
>
> void
> set_delayed_call(struct delayed_call *call, union delayed_call_fn u, void *arg)
> {
> 	call->fn = u.fn;
> 	call->arg = arg;
> }

	Yecchhhh...  If we are into that kind of gccisms, I'd rather use
__builtin_choose_expr/__builtin_types_compatible_p.  At least that is
used elsewhere in the kernel; __transparent_union__ kludge isn't.  Sure, it
means having set_delayed_call() a macro, but IMO it's less nasty that way...

	FWIW, another possibility is to have
#define CLOSURE_CALLBACK(f,type) \
	static inline void __closure_##f(void *p) {return f((type)p);}

#define set_delayed_type(call, f, arg) \
	sizeof(f(arg),0), \
	__set_delayed_type(call, __closure_##f, (void *)arg)

That could be reused for timers with typechecking - we have a lot of timer
callbacks that start with casting the argument (unsigned long, not void *,
but that's not a big deal) to whatever it is that callback really wants,
with setup_timer() callers explicitly casting that whatever the callback
really wants to unsigned long.  Which, of course, defeats the typechecking
by both cc(1) and sparse(1)...

I still hope for better solution, though...  Comments?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1290065

On Fri, Dec 11, 2015 at 11:16:24PM +0000, Al Viro wrote:

> #define set_delayed_type(call, f, arg) \
> 	sizeof(f(arg),0), \
> 	__set_delayed_type(call, __closure_##f, (void *)arg)
> 
> That could be reused for timers with typechecking - we have a lot of timer
> callbacks that start with casting the argument (unsigned long, not void *,
> but that's not a big deal) to whatever it is that callback really wants,
> with setup_timer() callers explicitly casting that whatever the callback
> really wants to unsigned long.  Which, of course, defeats the typechecking
> by both cc(1) and sparse(1)...
> 
> I still hope for better solution, though...  Comments?

Hmm...

#define set_delayed_type(call, f, arg)					\
 	(sizeof(f(arg),0),						\
	__set_delayed_type(call, ({					\
		void _(void *__) {f((typeof(arg))(unsigned long)__);}	\
		_;}), (void *)arg))

woult do nicely, but it's a gccism, and the one clang doesn't support ;-/
gcc support goes back at least to 4.1, not sure about the earlier branches
(and not at all sure how flaky it was/is, obviously).

But this thing works for e.g. f being void(char) and arg being char,
and AFAICS generates correct code - we end up with a stub like
_.1765:
        movsbl  %dil, %edi
        jmp     f
and its address gets stored in call->fn, which should DTRT on call...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290668

On Sat, Dec 12 2015, Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Fri, Dec 11, 2015 at 11:16:24PM +0000, Al Viro wrote:
>
>> #define set_delayed_type(call, f, arg) \
>> 	sizeof(f(arg),0), \
>> 	__set_delayed_type(call, __closure_##f, (void *)arg)
>> 
>> That could be reused for timers with typechecking - we have a lot of timer
>> callbacks that start with casting the argument (unsigned long, not void *,
>> but that's not a big deal) to whatever it is that callback really wants,
>> with setup_timer() callers explicitly casting that whatever the callback
>> really wants to unsigned long.  Which, of course, defeats the typechecking
>> by both cc(1) and sparse(1)...
>> 
>> I still hope for better solution, though...  Comments?
>
> Hmm...
>
> #define set_delayed_type(call, f, arg)					\
>  	(sizeof(f(arg),0),						\
> 	__set_delayed_type(call, ({					\
> 		void _(void *__) {f((typeof(arg))(unsigned long)__);}	\
> 		_;}), (void *)arg))
>
> woult do nicely, but it's a gccism, and the one clang doesn't support
> ;-/

Careful. While this may appear to work, I'm pretty sure it would break
horribly when f is not the actual name of a function but a function
pointer. Not that it won't compile, but it would presumably make gcc
emit what they call a 'trampoline', a small piece of executable code
located on the stack, allowing the stub to access the containing
function's local variables (namely, f). I don't know if kernel stacks
are even executable, but even then it's a bad idea, once the stack frame
containing the trampoline is gone. All of this could easily happen down
the line when someone sees a bunch of similar code and decides to factor
that into a helper(), and then it works whenever gcc decides to inline
the helper to both call sites and otherwise, as the gcc docs put it,
"all hell breaks loose". IOWs, an accident waiting to happen.

One could probably ensure that the macro is always used with a
compile/link-time constant by putting in some trick like

static void *link_time_constant_please = f;

but even then I wouldn't be completely sure that gcc would always emit a
stub as simple as the one you showed.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web