Re: [PATCH] delay-accounting: Re-implement -c for getdelays.c toreport information on a target command

[Mel Gorman]

On Tue, Jan 19, 2010 at 11:43:43PM +0530, Balbir Singh wrote:
> On Tuesday 19 January 2010 11:30 PM, Mel Gorman wrote:
> > Task delay-accounting was identified as one means of determining how long
> > a process spends in congestion_wait() without adding new statistics. For
> > example, if the workload should not be doing IO, delay-accounting could
> > reveal how long it was spending in unexpected IO or delays. Unfortunately,
> > on closer examination it was clear that getdelays does not act as documented.
> > 
> > Commit a3baf649 added Documentation/accounting/getdelays.c with a -c switch
> > that was documented to fork/exec a child and report statistics on it but for
> > reasons that are unclear to me, commit 9e06d3f9 deleted support for this
> 
> That is an oversight and not intentional. I've not gotten around to
> reimplementing the -c option due to lack for people asking for it.
> 

Consider this a request then. Man, I'd really like if someone
implemented that -c switch thing :P

> > switch but did not update the documentation. It might be an oversight or
> > it might be because the control flow of the program meant that accounting
> > information would be printed once early in the lifetime of the program
> > making it of limited use.
> > 
> > This patch reimplements -c for getdelays.c to act as documented. Unlike the
> > original version, it waits until the command completes before printing any
> > information on it. An example of it being used looks like
> > 
> 
> Looks good, could you please keep the original sign-offs as well?
> 

Well, the reimplementation is significantly different to what was there
so I'm not sure that's appropriate. Look at the differences yourself.

> > $ ./getdelays -d -c find /home/mel -name mel
> > print delayacct stats ON
> > /home/mel
> > /home/mel/.notes-wine/drive_c/windows/profiles/mel
> > /home/mel/.wine/drive_c/windows/profiles/mel
> > /home/mel/git-configs/dot.kde/share/apps/konqueror/home/mel
> > PID	5923
> > 
> > CPU             count     real total  virtual total    delay total
> >                 42779     5051232096     5164722692      564207988
> > IO              count    delay total
> >                 41727    97804147758
> > SWAP            count    delay total
> >                     0              0
> > RECLAIM         count    delay total
> >                     0              0
> > 
> > It's not clear how or if this subsystem is being maintained. If the
> > authors agree on it but do not pick it up for merging, I'll go bug
> > Andrew with it.
> > 
> 
> I am maintaining it, please do let me know if you have an issue with the
> subsystem or utilities.
> 

If you're happy with the patch then, can you pick it up, add your
signed-off-by and go with whatever submission path you use for this
subsystem?

Thanks

> > Signed-off-by: Mel Gorman <mel@xxxxxxxxx>
> > --- 
> >  Documentation/accounting/getdelays.c |   37 +++++++++++++++++++++++++++++++++--
> >  1 file changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/accounting/getdelays.c b/Documentation/accounting/getdelays.c
> > index 6e25c26..c6648bc 100644
> > --- a/Documentation/accounting/getdelays.c
> > +++ b/Documentation/accounting/getdelays.c
> > @@ -21,6 +21,7 @@
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <sys/socket.h>
> > +#include <sys/wait.h>
> >  #include <signal.h>
> > 
> >  #include <linux/genetlink.h>
> > @@ -266,11 +267,13 @@ int main(int argc, char *argv[])
> >  	int containerset = 0;
> >  	char containerpath[1024];
> >  	int cfd = 0;
> > +	int forking = 0;
> > +	sigset_t sigset;
> > 
> >  	struct msgtemplate msg;
> > 
> > -	while (1) {
> > -		c = getopt(argc, argv, "qdiw:r:m:t:p:vlC:");
> > +	while (!forking) {
> > +		c = getopt(argc, argv, "qdiw:r:m:t:p:vlC:c:");
> >  		if (c < 0)
> >  			break;
> > 
> > @@ -319,6 +322,27 @@ int main(int argc, char *argv[])
> >  				err(1, "Invalid pid\n");
> >  			cmd_type = TASKSTATS_CMD_ATTR_PID;
> >  			break;
> > +		case 'c':
> > +
> > +			/* Block SIGCHLD for sigwait() later */
> > +			if (sigemptyset(&sigset) == -1)
> > +				err(1, "Failed to empty sigset");
> > +			if (sigaddset(&sigset, SIGCHLD))
> > +				err(1, "Failed to set sigchld in sigset");
> > +			sigprocmask(SIG_BLOCK, &sigset, NULL);
> > +
> > +			/* fork/exec a child */
> > +			tid = fork();
> > +			if (tid < 0)
> > +				err(1, "Fork failed\n");
> > +			if (tid == 0)
> > +				if (execvp(argv[optind - 1], &argv[optind - 1]) < 0)
> > +					exit(-1);
> > +
> > +			/* Set the command type and avoid further processing */
> > +			cmd_type = TASKSTATS_CMD_ATTR_PID;
> > +			forking = 1;
> > +			break;
> >  		case 'v':
> >  			printf("debug on\n");
> >  			dbg = 1;
> > @@ -370,6 +394,15 @@ int main(int argc, char *argv[])
> >  		goto err;
> >  	}
> > 
> > +	/*
> > +	 * If we forked a child, wait for it to exit. Cannot use waitpid()
> > +	 * as all the delicious data would be reaped as part of the wait 
> > +	 */
> > +	if (tid && forking) {
> > +		int sig_received;
> > +		sigwait(&sigset, &sig_received);
> > +	}
> > +
> >  	if (tid) {
> >  		rc = send_cmd(nl_sd, id, mypid, TASKSTATS_CMD_GET,
> >  			      cmd_type, &tid, sizeof(__u32));
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/