* types for storing instruction pointers
@ 2009-06-04 13:14 Christoph Hellwig
2009-06-07 13:25 ` Ingo Molnar
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2009-06-04 13:14 UTC (permalink / raw)
To: Ingo Molnar, Frederic Weisbecker; +Cc: linux-kernel
Currently the _THIS_IP_ and _RET_IP_ macros aded for lockdep but now
available from kernel.org case the instruction pointer to an unsigned
long. But the %pf/%pF format for printing them want a pointer of some
sort. That's a pretty nasty situation for tracers - can we standardize
on one type for it?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: types for storing instruction pointers
2009-06-04 13:14 types for storing instruction pointers Christoph Hellwig
@ 2009-06-07 13:25 ` Ingo Molnar
2009-06-08 13:51 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Ingo Molnar @ 2009-06-07 13:25 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Frederic Weisbecker, linux-kernel
* Christoph Hellwig <hch@lst.de> wrote:
> Currently the _THIS_IP_ and _RET_IP_ macros aded for lockdep but
> now available from kernel.org case the instruction pointer to an
> unsigned long. But the %pf/%pF format for printing them want a
> pointer of some sort. That's a pretty nasty situation for tracers
> - can we standardize on one type for it?
Yes but what's the practical problem exactly? Could you cite an
example?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: types for storing instruction pointers
2009-06-07 13:25 ` Ingo Molnar
@ 2009-06-08 13:51 ` Christoph Hellwig
2009-06-08 14:12 ` Ingo Molnar
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2009-06-08 13:51 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Christoph Hellwig, Frederic Weisbecker, linux-kernel
On Sun, Jun 07, 2009 at 03:25:16PM +0200, Ingo Molnar wrote:
>
> * Christoph Hellwig <hch@lst.de> wrote:
>
> > Currently the _THIS_IP_ and _RET_IP_ macros aded for lockdep but
> > now available from kernel.org case the instruction pointer to an
> > unsigned long. But the %pf/%pF format for printing them want a
> > pointer of some sort. That's a pretty nasty situation for tracers
> > - can we standardize on one type for it?
>
> Yes but what's the practical problem exactly? Could you cite an
> example?
The simplest tracer in xfs showing this is the following:
/*
* ilock/iolock tracer
*
* Reports the inode, operation, flags and caller for each operation
* on the inode locks.
*/
TRACE_EVENT(xfs_ilock,
TP_PROTO(struct xfs_inode *ip, const char *op, unsigned lockflags,
unsigned long caller_ip),
TP_ARGS(ip, op, lockflags, caller_ip),
TP_STRUCT__entry(
__field(dev_t, dev)
__field(xfs_ino_t, ino)
__field(const char *, op)
__field(int, lockflags)
__field(unsigned long, caller_ip)
),
TP_fast_assign(
__entry->dev = VFS_I(ip)->i_sb->s_dev;
__entry->ino = ip->i_ino;
__entry->op = op;
__entry->lockflags = lockflags;
__entry->caller_ip = caller_ip;
),
TP_printk("dev %d:%d ino 0x%lld %s %s by %pF",
MAJOR(__entry->dev), MINOR(__entry->dev),
__entry->ino,
__entry->op,
__print_flags(__entry->lockflags, "|", XFS_LOCK_FLAGS),
(void *)__entry->caller_ip)
);
It has the following callers:
trace_xfs_ilock(ip, "lock", lock_flags, _RET_IP_);
trace_xfs_ilock(ip, "lock_nowait", lock_flags, _RET_IP_);
trace_xfs_ilock(ip, "unlock", lock_flags, _RET_IP_);
trace_xfs_ilock(ip, "demote", lock_flags, _RET_IP_);
Basically everything obtained via _RET_IP_/_THIS_IP_ needs to be
casted. Given that both need to case the their return value to a
pointer that's rather unfortunately. Life would be much easier
if _RET_IP_/_THIS_IP_ just returned a pointer (probably just a void
pointer, maybe with a fancy typedef to make it clear we're dealing
with an instruction pointer here).
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: types for storing instruction pointers
2009-06-08 13:51 ` Christoph Hellwig
@ 2009-06-08 14:12 ` Ingo Molnar
0 siblings, 0 replies; 4+ messages in thread
From: Ingo Molnar @ 2009-06-08 14:12 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Frederic Weisbecker, linux-kernel
* Christoph Hellwig <hch@lst.de> wrote:
> On Sun, Jun 07, 2009 at 03:25:16PM +0200, Ingo Molnar wrote:
> >
> > * Christoph Hellwig <hch@lst.de> wrote:
> >
> > > Currently the _THIS_IP_ and _RET_IP_ macros aded for lockdep but
> > > now available from kernel.org case the instruction pointer to an
> > > unsigned long. But the %pf/%pF format for printing them want a
> > > pointer of some sort. That's a pretty nasty situation for tracers
> > > - can we standardize on one type for it?
> >
> > Yes but what's the practical problem exactly? Could you cite an
> > example?
>
> The simplest tracer in xfs showing this is the following:
>
> /*
> * ilock/iolock tracer
> *
> * Reports the inode, operation, flags and caller for each operation
> * on the inode locks.
> */
> TRACE_EVENT(xfs_ilock,
> TP_PROTO(struct xfs_inode *ip, const char *op, unsigned lockflags,
> unsigned long caller_ip),
> TP_ARGS(ip, op, lockflags, caller_ip),
>
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(xfs_ino_t, ino)
> __field(const char *, op)
> __field(int, lockflags)
> __field(unsigned long, caller_ip)
> ),
>
> TP_fast_assign(
> __entry->dev = VFS_I(ip)->i_sb->s_dev;
> __entry->ino = ip->i_ino;
> __entry->op = op;
> __entry->lockflags = lockflags;
> __entry->caller_ip = caller_ip;
> ),
>
> TP_printk("dev %d:%d ino 0x%lld %s %s by %pF",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> __entry->ino,
> __entry->op,
> __print_flags(__entry->lockflags, "|", XFS_LOCK_FLAGS),
> (void *)__entry->caller_ip)
> );
>
> It has the following callers:
>
> trace_xfs_ilock(ip, "lock", lock_flags, _RET_IP_);
> trace_xfs_ilock(ip, "lock_nowait", lock_flags, _RET_IP_);
> trace_xfs_ilock(ip, "unlock", lock_flags, _RET_IP_);
> trace_xfs_ilock(ip, "demote", lock_flags, _RET_IP_);
>
> Basically everything obtained via _RET_IP_/_THIS_IP_ needs to be
> casted. Given that both need to case the their return value to a
> pointer that's rather unfortunately. Life would be much easier if
> _RET_IP_/_THIS_IP_ just returned a pointer (probably just a void
> pointer, maybe with a fancy typedef to make it clear we're dealing
> with an instruction pointer here).
Yeah, there's really no coherency in this area anywhere in the
kernel. IPs are often represented as unsigned long, in symbol lookup
and elsewhere - that is where lockdep got that principle from. There
it hurts if we do this change - i.e. we'd just shift the point of
'friction' between types from tracing to the symbol lookup code and
to platform code.
So i'd fully agree with making all that a (void *), but then you
need to hunt down all the other uses of code addresses as well and
standardize the thing all across the kernel. Not a small patch :-)
( Plus turning it into a separate type probably makes sense as well,
as there are platforms where function pointers are different from
regular pointers so keeping them sorted separate is good. )
Ingo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-06-08 14:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-04 13:14 types for storing instruction pointers Christoph Hellwig
2009-06-07 13:25 ` Ingo Molnar
2009-06-08 13:51 ` Christoph Hellwig
2009-06-08 14:12 ` Ingo Molnar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.