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

[Crystal Wood <swood@redhat.com>]

On Tue, 2022-12-13 at 14:31 -0500, John Kacur wrote:
> 
> 
> On Fri, 9 Dec 2022, Crystal Wood wrote:
> 
> > On Fri, 2022-12-09 at 20:03 -0500, John Kacur wrote:
> > 
> > > 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

I see it in a lot of the structure definitions and macros; looks like
checkpatch only cares if it's part of the whitespace at the beginning of a
line (which is not necessarily the same as indentation, but whatever).  I'll
change it since you asked (it wasn't clear from the docs that checkpatch.pl
is the applicable style guide), but barring specific style guides or
pushback I tend to default to being kind to people using different tab
widths, having been one in a past life before the kernel beat it out of me.
:-)

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

Sure.  As an aside, I do think that's a rule that is usually helpful but can
harm readability in some circumstances when applied strictly (e.g. "2*x + 1"
looks better to me than "2 * x + 1" which doesn't emphasize the order of
operations and doesn't provide the eye with as much structure).

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

Yes, that works and could be useful if you're trying to focus on larger
latency sources.

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

Anything that isn't a multiple of 1000 will give you nanosecond precision.

-Crystal