| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1216132928.12595.201.camel@twins>
Date: Tue, 15 Jul 2008 16:42:08 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc: akpm@...ux-foundation.org, Ingo Molnar <mingo@...e.hu>,
linux-kernel@...r.kernel.org,
Masami Hiramatsu <mhiramat@...hat.com>,
"Frank Ch. Eigler" <fche@...hat.com>,
Hideo AOKI <haoki@...hat.com>,
Takashi Nishiie <t-nishiie@...css.fujitsu.com>,
Steven Rostedt <rostedt@...dmis.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>,
Paul E McKenney <paulmck@...ux.vnet.ibm.com>
Subject: Re: [patch 01/15] Kernel Tracepoints
On Tue, 2008-07-15 at 10:27 -0400, Mathieu Desnoyers wrote:
> * Peter Zijlstra (peterz@...radead.org) wrote:
> > On Tue, 2008-07-15 at 09:25 -0400, Mathieu Desnoyers wrote:
> > > * Peter Zijlstra (peterz@...radead.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)
Ah, D'0h! Indeed.
> 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 ?
I'm confused by the barrier games here.
Why not:
void **it_func;
preempt_disable();
it_func = rcu_dereference((tp)->funcs);
if (it_func) {
for (; *it_func; it_func++)
((void(*)(proto))(*it_func))(args);
}
preempt_enable();
That is, why can we skip the barrier when !it_func? is that because at
that time we don't actually dereference it_func and therefore cannot
observe stale data?
If so, does this really matter since we're already in an unlikely
section? Again, if so, this deserves a comment ;-)
[ still think those preempt_* calls should be called
rcu_read_sched_lock() or such. ]
Anyway, does this still generate better code?
--
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