Re: [patch 01/15] Kernel Tracepoints

From: Mathieu Desnoyers
Date: Tue Jul 15 2008 - 10:32:24 EST


* Peter Zijlstra (peterz@xxxxxxxxxxxxx) wrote:
> On Tue, 2008-07-15 at 09:25 -0400, Mathieu Desnoyers wrote:
> > * Peter Zijlstra (peterz@xxxxxxxxxxxxx) wrote:
> > > On Wed, 2008-07-09 at 10:59 -0400, Mathieu Desnoyers wrote:
>
> > > > +#define __DO_TRACE(tp, proto, args) \
> > > > + do { \
> > > > + int i; \
> > > > + void **funcs; \
> > > > + preempt_disable(); \
> > > > + funcs = (tp)->funcs; \
> > > > + smp_read_barrier_depends(); \
> > > > + if (funcs) { \
> > > > + for (i = 0; funcs[i]; i++) { \
> > >
> > > can't you get rid of 'i' and write:
> > >
> > > void **func;
> > >
> > > preempt_disable();
> > > func = (tp)->funcs;
> > > smp_read_barrier_depends();
> > > for (; func; func++)
> > > ((void (*)(proto))func)(args);
> > > preempt_enable();
> > >
> >
> > Yes, I though there would be an optimization to do here, I'll use your
> > proposal. This code snippet is especially important since it will
> > generate instructions near every tracepoint side. Saving a few bytes
> > becomes important.
> >
> > Given that (tp)->funcs references an array of function pointers and that
> > it can be NULL, the if (funcs) test must still be there and we must use
> >
> > #define __DO_TRACE(tp, proto, args) \
> > do { \
> > void *func; \
> > \
> > preempt_disable(); \
> > if ((tp)->funcs) { \
> > func = rcu_dereference((tp)->funcs); \
> > for (; func; func++) { \
> > ((void(*)(proto))(func))(args); \
> > } \
> > } \
> > preempt_enable(); \
> > } while (0)
> >
> >
> > The resulting assembly is a bit more dense than my previous
> > implementation, which is good :
>
> My version also has that if ((tp)->funcs), but its hidden in the
> for (; func; func++) loop. The only thing your version does is an extra
> test of tp->funcs but without read depends barrier - not sure if that is
> ok.
>

Hrm, you are right, the implementation I just proposed is bogus. (but so
was yours) ;)

func is an iterator on the funcs array. My typing of func is thus wrong,
it should be void **. Otherwise I'm just incrementing the function
address which is plain wrong.

The read barrier is included in rcu_dereference() now. But given that we
have to take a pointer to the array as an iterator, we would have to
rcu_dereference() our iterator multiple times and then have many read
barrier depends, which we don't need. This is why I would go back to a
smp_read_barrier_depends().

Also, I use a NULL entry at the end of the funcs array as an end of
array identifier. However, I cannot use this in the for loop both as a
check for NULL array and check for NULL array element. This is why a if
() test is needed in addition to the for loop test. (this is actually
what is wrong in the implementation you proposed : you treat func both
as a pointer to the function pointer array and as a function pointer)

Something like this seems better :

#define __DO_TRACE(tp, proto, args) \
do { \
void **it_func; \
\
preempt_disable(); \
it_func = (tp)->funcs; \
if (it_func) { \
smp_read_barrier_depends(); \
for (; *it_func; it_func++) \
((void(*)(proto))(*it_func))(args); \
} \
preempt_enable(); \
} while (0)

What do you think ?

Mathieu

>
>

--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/