Message-ID: <20100310190330.GB5000@nowhere>
Date:	Wed, 10 Mar 2010 20:03:35 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Jason Baron <jbaron@...hat.com>
Cc:	mingo@...e.hu, rostedt@...dmis.org, linux-kernel@...r.kernel.org,
	laijs@...fujitsu.com, lizf@...fujitsu.com, hpa@...or.com,
	tglx@...utronix.de, mhiramat@...hat.com, heiko.carstens@...ibm.com,
	benh@...nel.crashing.org, davem@...emloft.net, lethal@...ux-sh.org,
	schwidefsky@...ibm.com, brueckner@...ux.vnet.ibm.com,
	tony.luck@...el.com
Subject: Re: [PATCH 00/12] tracing: add compat syscall support v2

On Fri, Feb 26, 2010 at 04:36:55PM -0500, Jason Baron wrote:
> Hi,
> 
> Re-post to add infrastructure for compat syscall event tracing support. This
> patch series also adds x86_64 arch specific support as an example consumer
> of the new infrastructure.
> 
> The new interface consists of:
> 
> 1) int is_compat_task(void);
>   - most arches seem to have this already
> 2) unsigned long arch_compat_syscall_addr(int nr);
>   - returns a pointer to the compat syscall entry corresponding to syscall 'nr'
> 3) int NR_syscalls_compat;
>   - number of entries in the compat syscall table.
> 
> Thus, arches that set CONFIG_FTRACE_SYSCALLS and CONFIG_COMPAT are going to
> need to implement the above interfaces in order to build. Thus, I'm 'cc arch
> maintainers.
> 
> Naming. I've also introduced a couple of new syscall macros:
> 
> ARCH_COMPAT_SYSCALL_DEFINE#N()
> COMPAT_SYSCALL_DEFINE#N()
> 
> These tack on, "arch_compat_sys" and "compat_sys" respectively, to the
> beginning of the compat syscall names.
> 
> thanks,
> 
> -Jason



That's a very nice work!

Some neats about it, some of them can be probably done
incrementally:


- The new COMPAT_SYSCALL_DEFINE/ARCH_COMPAT_SYSCALL_DEFINE
macros nicely standardize the syscalls naming. Most arch use
the sys32_* prefix for compat though. We could simply make
ARCH_COMPAT_SYSCALL_DEFINE() prepend with sys32_* as this
will involve less changes in the syscall table of archs that
want the compat syscall tracing support. (Although in itself,
having arch_compat_ prefixes is also an intuitive naming)

- Why bothering with a trace event namespace separation between
arch overriden compat syscalls and common ones?
Tools want to deal with a single shot of syscalls:compat_*, and
not syscalls:compat_* + syscalls:arch_compat_*
Even in the trace, such a naming separation may look weird.
The *_SYSCALL_DEFINE macros can make it easy to use different
names between syscall functions and syscall trace event names.
So, we can keep the distinct arch_compat (or sys32 that involves
less changes) and compat prefixes for functions names, as arch_compat
may call compat things; and have only compat prefixes as trace events
names. And may be if we have collisions between arch and generic
compat callback names, let's drop the generic one as it will
never trigger, since it's not in the syscall table anyway.

- We probably want a compat_syscalls trace event subsystem (separated
from syscalls), can be done incrementally though

- Commit e7b8e675d9c71b868b66f62f725a948047514719
  (tracing: Unify arch_syscall_addr() implementations)
has made a good use of the unified syscall table name
between archs that support syscall tracing so that we
can, most of the time, directly dereference it from generic
code. Exotic archs can still override arch_syscall_addr()
if we can't do that with their syscall table.
I wish we can do that with the compat syscall tables too.
Compat syscall tables names seem to be less unified for now
though. Again, can be done incrementally.

Other than that, that's a cool batch!

Thanks!



> 
> Jason Baron (11):
>   x86: add NR_syscalls_compat, make ia32 syscall table visible
>   x86: add arch_compat_syscall_addr()
>   tracing: remove syscall bitmaps in preparation for compat support
>   tracing: add tracing support for compat syscalls
>   syscalls: add ARCH_COMPAT_SYSCALL_DEFINE()
>   x86, compat: convert ia32 layer to use ARCH_COMPAT_SYSCALL_DEFINE#N()
>   syscalls: add new COMPAT_SYSCALL_DEFINE#N() macro
>   compat: convert to use COMPAT_SYSCALL_DEFINE#N()
>   compat: convert fs compat to use COMPAT_SYSCALL_DEFINE#N() macros
>   tags: recognize syscalls
>   cleanup: remove arg from TRACE_SYS_ENTER_PROFILE_INIT() macro
> 
> Heiko Carstens (1):
>   compat: have generic is_compat_task for !CONFIG_COMPAT
> 
>  arch/s390/include/asm/compat.h  |    7 --
>  arch/s390/kernel/ptrace.c       |    2 +-
>  arch/s390/kernel/setup.c        |    2 +-
>  arch/s390/mm/mmap.c             |    2 +-
>  arch/x86/ia32/ia32entry.S       |   53 ++++++++-------
>  arch/x86/ia32/sys_ia32.c        |  106 ++++++++++++++--------------
>  arch/x86/include/asm/compat.h   |    2 +
>  arch/x86/kernel/ftrace.c        |   11 +++
>  drivers/s390/block/dasd_eckd.c  |    2 +-
>  drivers/s390/block/dasd_ioctl.c |    1 +
>  drivers/s390/char/fs3270.c      |    1 +
>  drivers/s390/char/vmcp.c        |    1 +
>  drivers/s390/cio/chsc_sch.c     |    1 +
>  drivers/s390/scsi/zfcp_cfdc.c   |    1 +
>  fs/compat.c                     |  147 +++++++++++++++++++--------------------
>  include/linux/compat.h          |    9 +++
>  include/linux/syscalls.h        |   70 ++++++++++++-------
>  include/trace/syscall.h         |    8 ++
>  kernel/compat.c                 |  106 ++++++++++++++---------------
>  kernel/trace/trace.h            |    2 +
>  kernel/trace/trace_syscalls.c   |  101 +++++++++++++++++++--------
>  scripts/tags.sh                 |    8 ++-
>  22 files changed, 369 insertions(+), 274 deletions(-)
> 

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