On Wed, Feb 16, 2005 at 06:45:02AM -0800, Chuck Silvers wrote:
> On Wed, Feb 16, 2005 at 07:42:13AM +0000, Nick Hudson wrote:
> > Folks,
> > 
> > I'm at a point where I'd like to merge ktrace-lwp sometime soon. As yamt-km is 
> > also ready for merge I'll delay my merge until a few days after yamt-km has 
> > gone in.
> > 
> > Any comments on the branch and/or the merge plan?
> 
> hi,
> 
> could you post a summary of what major interfaces are changing?
> (ie. anything that would have warranted updating the kernel version.)
> I assume they're mostly just changing "struct proc" to "struct lwp",
> but it would be nice to know where all that had to happen, and if there
> were any other interface changes required.  also, I saw your most recent
> checkins had to do with the file format, could you describe how that
> is changing as well?


nick replied privately that he hasn't had time to do this, so I took a
stab at it myself.  I looked through header files for changes to data
fields or to function pointer arguments, and it appeared that almost all
of the changes can be summarized that way.  here's the list:


changes to arguments of callbacks/methods:

struct bdevsw
d_open
d_close
d_ioctl

struct cdevsw
d_open
d_close
d_ioctl
d_poll

struct domain
dom_externalize

typedef exec_makecmds_fcn

struct exec_vmcmd
ev_proc

struct execsw
  elf_probe_func
  ecoff_probe_func
es_copyargs
es_setup_stack

struct fbdriver
fbd_open
fbd_close
fbd_ioctl
fbd_poll

struct file
fo_ioctl
fo_fcntl
fo_poll
fo_stat
fo_close

struct mount
VFS_MOUNT
VFS_START
VFS_UNMOUNT
VFS_QUOTACTL
VFS_STATVFS
VFS_SYNC
VFS_EXTATTRCTL

struct protosw
pr_usrreq

struct socket
so_send

struct vnode
VOP_OPEN
VOP_CLOSE
VOP_ACCESS
VOP_GETATTR
VOP_SETATTR
VOP_IOCTL
VOP_FCNTL
VOP_POLL
VOP_MMAP
VOP_FSYNC
VOP_ABORTOP
VOP_INACTIVE
VOP_RECLAIM
VOP_TRUNCATE
VOP_LEASE
VOP_CLOSEEXTATTR
VOP_GETEXTATTR
VOP_LISTEXTATTR
VOP_LISTEXTATTR
VOP_DELETEEXTATTR
VOP_SETEXTATTR

(there were some more open/close/ioctl type methods that I left out
since they was the same as the above)


I didn't look to see how many of these really needed the lwp vs. the proc,
but it does seem reasonable that most of them would.

some operations that are more an internal detail rather than something that
is some explicitly (eg. VOP_INACTIVE, VOP_RECLAIM, and VOP_ABORTOP) may not
need to know either the proc or the lwp, so it would be nice to see if we
can just remove the argument from those.  for other ones like VFS_SYNC,
I don't think it's going to anything differently depending on who the caller
is, so those might not need to know either.




data field changes:

struct mount
mnt_unmounter

struct componentname
cn_proc (renamed to cn_lwp)

struct uio
uio_procp (renamed to uio_lwp)


all of these changes were, as one would expect, converting "struct proc *"
to "struct lwp *".

I think that uio_procp was originally used more to refer to the address space
than the thread, so it would be good to take a look at which (if any) paths
actually need the lwp pointer and see if they can get it some other way.
I took a quick look at this and almost all of the uses are just to get at
the proc.

most of the users of cn_lwp look like they really do want the lwp,
so that one is probably a good change.

mnt_unmounter is used in about 3 places total, so I don't care about
that one.  :-)



as for the layout changes, we appear to be reinterpreting some of the
fields and bits in struct ktr_header.

old:

struct ktr_header {
	int	ktr_len;		/* length of ktr_buf */
	short	ktr_type;		/* trace record type */
	pid_t	ktr_pid;		/* process id */
	char	ktr_comm[MAXCOMLEN+1];	/* command name */
	struct	timeval ktr_time;	/* timestamp */
	const void *ktr_buf;
};


new:

struct ktr_header {
        int     ktr_len;                /* length of record minus length of old header */
#if BYTE_ORDER == LITTLE_ENDIAN
        short   ktr_type;               /* trace record type */
        short   ktr_version;            /* trace record version */
#else
        short   ktr_version;            /* trace record version */
        short   ktr_type;               /* trace record type */
#endif
        pid_t   ktr_pid;                /* process id */
        char    ktr_comm[MAXCOMLEN+1];  /* command name */
        union {
                struct timeval _tv;     /* v0 timestamp */
                struct timespec _ts;    /* v1 timespec */
        } _ktr_time;
        union {
                const void *_buf;       /* v0 unused */
                lwpid_t _lid;           /* v1 lwp id */
        } _ktr_id;
};


so we're re-using the implicit padding between ktr_type and ktr_pid to store
the record version.  that looks fine.  but since we're changing the layout
it's be nice to take the opportunity to make it common across platforms.
the use of "struct timeval" (whose members are of type "long") makes the
files different on different platforms.  old kdump binaries won't be
able to read the new files correctly anyway, so there's no need to make
the records the same size.  I listed some specifics under "suggestions" below.


I also noticed what appear to be some merge botches of copyright years.
in these two files, some years have been removed from the list:

arch/hpc/stand/hpcboot/hpcboot.h
arch/hpc/stand/hpcboot/hpcmenu.h

these should be fixed, and we should look to see if there are other instances.
grepping for "Copyright" in "cvs diff -r ktrace-lwp-base -r ktrace-lwp",
it looks like there are a few more.


I also noticed there are still a references to uio_procp.
if we don't end up changing this back, then we should convert this one as well.
it's in dev/ata/wd.c (I found it with cscope).



so to summarize my suggestions:

 - reconsider whether uio_procp really needs to change.
 - investigate eliminating proc/lwp args from some APIs.
 - consider using all fixed-size types for ktr_header (ie. uint*_t).
   so that trace files are more portable.
   - get rid of the pointer from the on-disk format
   - timeval/timespec have "long", so don't use them directly.
     choose 32- or 64-bit and use them explicitly.
   - if there's also some way to detect the endianness, then the v1 layout
     should be completely MI after these changes.
 - fix copyright merge botches.
 - if we stick with uio_lwp, fix the uio_procp references that were missed.


-Chuck