Re: [PATCH] oslat: Add command line option for bucket width

[John Kacur <jkacur@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]



On Tue, 13 Dec 2022, John Kacur wrote:

> 
> 
> On Fri, 9 Dec 2022, Crystal Wood wrote:
> 
> > On Fri, 2022-12-09 at 20:03 -0500, John Kacur wrote:
> > > 
> > > 
> > > On Thu, 8 Dec 2022, Crystal Wood wrote:
> > > 
> > > 
> > > >  
> > > > @@ -677,6 +699,20 @@ static void parse_options(int argc, char *argv[])
> > > >                                 exit(1);
> > > >                         }
> > > >                         break;
> > > > +               case OPT_BUCKETWIDTH:
> > > > +               case 'W':
> > > > +                       g.bucket_width = strtol(optarg, NULL, 10);
> > > > +                       if (g.bucket_size <= 0) {
> > > 
> > > I think this should be g.bucket_width
> > 
> > Oops
> > 
> > > 
> > > A quick first look through and run, see the one comment above
> > > near "case 'W'"
> > > 
> > > and then
> > > 
> > > checkpatch reports some minor easily fixed problems
> > > 
> > > ../linux/scripts/checkpatch.pl oslat.patch 
> > > ERROR: code indent should use tabs where possible
> > > #100: FILE: src/oslat/oslat.c:342:
> > > +^I^I          g.precision, us);$
> > > 
> > > ERROR: code indent should use tabs where possible
> > > #102: FILE: src/oslat/oslat.c:344:
> > > +^I^I         g.precision, us);$
> > 
> > I was matching the existing style in the file that tended to use spaces for
> > alignment.
> > 
> > > ERROR: spaces required around that '=' (ctx:VxV)
> > > #227: FILE: src/oslat/oslat.c:654:
> > > +       OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, 
> > > OPT_CPU_MAIN_THREAD,
> > >                       ^
> > 
> > I only added OPT_BUCKETWIDTH to the list; I didn't touch the =1 part.
> > 
> > -Scott
> > 
> > 
> 
> One more thing that we just saw, if you run oslat without any options, 
> just the defaults you get 32 buckets with the highest one being 32us
> 
> But if you run
> oslat -W 500
> 
> You still get 32 buckets but since the width is half, then largest bucket
> is 32us

I mean 16us of course

> 
> This increases the resolution of the buckets, but it puts all the overflow
> in the 16us buckets, wondering if we should double the number of buckets
> so that the largest one is still 32us ?
> 
> I realize you could do
> oslat -b 64  -W 500
> to achieve that, but perhaps the default is not good like this.
> 
> John