Messages in this thread | | | Date | Tue, 15 Jul 2008 10:27:11 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [patch 01/15] Kernel Tracepoints |
| |
* Peter Zijlstra (peterz@infradead.org) wrote: > On Tue, 2008-07-15 at 09:25 -0400, Mathieu Desnoyers wrote: > > * Peter Zijlstra (peterz@infradead.org) 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
| |