types for storing instruction pointers

types for storing instruction pointers

[Christoph Hellwig]

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?

Re: types for storing instruction pointers

[Ingo Molnar]

* 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

Re: types for storing instruction pointers

[Christoph Hellwig]

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).

Re: types for storing instruction pointers

[Ingo Molnar]

* 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