Re: [PATCH] cyclictest: make clock_nanosleep as default and add option for POSIX timers.

["Patel, Vedang" <vedang.patel@intel.com>]

On Thu, 2017-03-23 at 20:14 +0100, Henrik Austad wrote:
> On Tue, Mar 14, 2017 at 12:43:48PM -0700, Vedang Patel wrote:
> > 
> > it is recommended that clock_nanosleep should be used for real-time
> extreme nitpick: Captialize first word in a sentence.
> > 
> > wherever available. So, make sure that cyclictest runs
> > clock_nanosleep
> > by default. Added an option to run POSIX timers. Removing the '-n'
> > option because it is redundant now.
> > 
> > Signed-off-by: Vedang Patel <vedang.patel@intel.com>
> > ---
> >  src/cyclictest/cyclictest.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/cyclictest/cyclictest.c
> > b/src/cyclictest/cyclictest.c
> > index 3f1bef1ffca9..cf82f8e1deaa 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -1321,7 +1321,6 @@ static void display_help(int error)
> >  	       "-m       --mlockall        lock current and future
> > memory allocations\n"
> >  	       "-M       --refresh_on_max  delay updating the
> > screen until a new max\n"
> >  	       "			   latency is hit. Userful
> > for low bandwidth.\n"
> > -	       "-n       --nanosleep       use clock_nanosleep\n"
> >  	       "	 --notrace	   suppress tracing\n"
> >  	       "-N       --nsecs           print results in ns
> > instead of us (default us)\n"
> >  	       "-o RED   --oscope=RED      oscilloscope mode,
> > reduce verbose output by RED\n"
> > @@ -1364,7 +1363,8 @@ static void display_help(int error)
> >  	       "                           format: n:c:v n=tasknum
> > c=count v=value in us\n"
> >  	       "-w       --wakeup          task wakeup tracing
> > (used with -b)\n"
> >  	       "-W       --wakeuprt        rt task wakeup tracing
> > (used with -b)\n"
> > -	       "	 --dbg_cyclictest  print info useful for
> > debugging cyclictest\n",
> > +	       "	 --dbg_cyclictest  print info useful for
> > debugging cyclictest\n"
> > +	       "-x	 --posix_timers    use POSIX timers\n",
> Since we are now removing all references to clock_nanosleep, perhaps
> add 
> note here indicating that nanosleep is the default, i.e. something
> like
> 
> "use POSIX timers instead of clock_nanosleep"
> 
> > 
> >  	       tracers
> >  		);
> >  	if (error)
> > @@ -1372,7 +1372,7 @@ static void display_help(int error)
> >  	exit(EXIT_SUCCESS);
> >  }
> >  
> > -static int use_nanosleep;
> > +static int use_nanosleep = MODE_CLOCK_NANOSLEEP; /* use
> > clock_nanosleep by default. */
> Not sure if you really need the trailing comment, but I'll leave that
> up to 
> John (I'd drop it)
> 
> > 
> >  static int timermode = TIMER_ABSTIME;
> >  static int use_system;
> >  static int priority;
> > @@ -1493,6 +1493,7 @@ enum option_values {
> >  	OPT_TRIGGER_NODES, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE,
> > OPT_WAKEUP,
> >  	OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP,
> > OPT_NUMOPTS,
> >  	OPT_ALIGNED, OPT_SECALIGNED, OPT_LAPTOP, OPT_SMI,
> > OPT_TRACEMARK,
> > +	OPT_POSIX_TIMERS,
> >  };
> >  
> >  /* Process commandline options */
> > @@ -1530,7 +1531,6 @@ static void process_options (int argc, char
> > *argv[], int max_cpus)
> >  			{"loops",            required_argument,
> > NULL, OPT_LOOPS },
> >  			{"mlockall",         no_argument,       NU
> > LL, OPT_MLOCKALL },
> >  			{"refresh_on_max",   no_argument,       NU
> > LL, OPT_REFRESH },
> > -			{"nanosleep",        no_argument,       NU
> > LL, OPT_NANOSLEEP },
> >  			{"nsecs",            no_argument,       NU
> > LL, OPT_NSECS },
> >  			{"oscope",           required_argument,
> > NULL, OPT_OSCOPE },
> >  			{"traceopt",         required_argument,
> > NULL, OPT_TRACEOPT },
> > @@ -1557,9 +1557,10 @@ static void process_options (int argc, char
> > *argv[], int max_cpus)
> >  			{"dbg_cyclictest",   no_argument,       NU
> > LL, OPT_DBGCYCLIC },
> >  			{"policy",           required_argument,
> > NULL, OPT_POLICY },
> >  			{"help",             no_argument,       NU
> > LL, OPT_HELP },
> > +			{"posix_timers",     no_argument,	N
> > ULL, OPT_POSIX_TIMERS },
> >  			{NULL, 0, NULL, 0}
> >  		};
> > -		int c = getopt_long(argc, argv,
> > "a::A::b:Bc:Cd:D:Efh:H:i:Il:MnNo:O:p:PmqrRsSt::uUvD:wWT:",
> > +		int c = getopt_long(argc, argv,
> > "a::A::b:Bc:Cd:D:Efh:H:i:Il:MNo:O:p:PmqrRsSt::uUvD:wWT:x",
> >  				    long_options, &option_index);
> >  		if (c == -1)
> >  			break;
> > @@ -1651,9 +1652,6 @@ static void process_options (int argc, char
> > *argv[], int max_cpus)
> >  		case 'M':
> >  		case OPT_REFRESH:
> >  			refresh_on_max = 1; break;
> > -		case 'n':
> > -		case OPT_NANOSLEEP:
> > -			use_nanosleep = MODE_CLOCK_NANOSLEEP;
> > break;
> >  		case 'N':
> >  		case OPT_NSECS:
> >  			use_nsecs = 1; break;
> > @@ -1760,6 +1758,9 @@ static void process_options (int argc, char
> > *argv[], int max_cpus)
> >  		case 'W':
> >  		case OPT_WAKEUPRT:
> >  			tracetype = WAKEUPRT; break;
> > +		case 'x':
> > +		case OPT_POSIX_TIMERS:
> > +			use_nanosleep = MODE_CYCLIC; break;
> >  		case '?':
> >  		case OPT_HELP:
> >  			display_help(0); break;
> As a side-observation that does not have anything to do with your
> patch, it 
> seems like there's a lovely mix of spaces and TABS in this file
> (i.e. 
> starting a line with a few spaces, then TABs and then possibly more
> spaces 
> for final alignment).
> 
> Could be my mailclient pulling my foot though, I just found it a bit 
> strange.
> 
> > 
> > -- 
> > 2.7.3
> > 
> A part form my minor nitpicks, nice change!
Thanks for the review Henrik! 

I agree with your suggestions. Will make the changes and send out new
version of the patch.

-Vedang
>