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

[John Kacur <jkacur@redhat.com>]

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



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.

Hmmn, when I run checkpatch on the file I don't get any other warnings 
about spaces, or when I look in vim I see tabs, maybe check your editor?
Please fix the above when you submit a version two

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

Yup, you're right, we're not as strict about the formatting in rt-tests as
the kernel is, and sure it will catch stuff that isn't strictly your fault 
but I do normally run checkpatch on the patches. If you want to throw in 
the spaces around the '=' in your patch I'd appreciate it.

Now to the more important stuff, I like your patch, but I have a few 
questions.

What if the user specifies a bucket-width greater than 1000?
Is it meaningful to to have a bucket-width of 2000 for example?

If we have a bucket width of 1500, then we are going to have 
the same precision as if we specified 500?

By the way I am not aware of any tools that are consuming
json output, and if there are, I'm sure they can adapt to
any changes such as decimal, so I wouldn't worry about that.

John