Re: [PATCH v2 0/9] Trace2 stopwatch timers and global counters

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:

I left some other comments on the series inline, just on the notes in
the CL:

>  * Ævar proposed a large refactor of the "_perf" target to have a "fmt()"
>    varargs function to reduce the amount of copy-n-pasted code in many of
>    the "fn" event handlers. This looks like a good change based on the
>    mockup but is a large refactor.

FWIW what I meant with [1] was not that this series needed to take the
detour of refactoring trace2/tr2_tgt_perf.c to use such a helper, but
that for the function additions in this series it might make sense to
introduce one and use it for the new functions.

For this series I think it's probably not worth it, so I'm fine with
leaving this for some other time. Just pointing out that rather than
your reading of:

 1. We have some refactorable verbosity
 2. Refactor all callers
 3. Change existing code to use that refactoring
 4. Add new code to use the refactoring

It's also perfectly fine to do just:

 1. We have some refactorable verbosity
 2. Introduce a less verbose
 3. Add new code to use the helper

And leave the "refactor all callers" for some other time.

Anyway, I think for the two callers just leaving it entirely for this
series is the right thing to do. It was more of a "hrm, that's some odd
and avoidable verbosity..." comment on me read-through of v1.

1. https://lore.kernel.org/git/211220.86czlrurm6.gmgdl@evledraar.gmail.com/

>  * Ævar proposed a new rationale for when/why we change the "_event" version
>    number. That text can be added to the design document independently.

Hrm, no. In [1] I linked to some earlier musings of mine about what we
should do about the TR2_EVENT_VERSION (mainly as an FYI since you added
it, but hadn't commented on that post).

But my main comment there was that the series wasn't progressing as
atomic changes. I.e. we promise to change the TR2_EVENT_VERSION version
every time we change the event format, but v1 first changed the format
and bumped the version, then made some more changes.

I think that's probably fine per-se within a git release cycle, but it
might be a symtom of commits that could be split up to be more atomic (I
don't know, didn't look in detail).

However, in this v2 of the series the TR2_EVENT_VERSION bump is entirely
gone.

Maybe that means that you so vehemently agree with my proposal in [1] it
that you'd like to start taking that view for trace2 changes right away
:-)

For me it's fine either way, I think TR2_EVENT_VERSION probably isn't
that important.

But if that's the case it should probably be called out more explictly
in the CL/commit. I.e. even if our "policy" (such as it is) about
TR2_EVENT_VERSION currently says X we're going to start doing Y here
intentionally.

And in that case I should probably turn that suggestion in [1] into a an
actual PATCH sooner than later...

1. https://lore.kernel.org/git/211220.86czlrurm6.gmgdl@evledraar.gmail.com/