RE: [PATCH V2 1/4] perf/x86/intel/uncore: use same idx for clinet IMC uncore events

[Liang, Kan]

> > To specially handle it, event->hw.idx >= UNCORE_PMC_IDX_FIXED is used
> to
> > check fixed counters in the generic uncore_perf_event_update.
> > It does not have problem in current code.
> 
> I disagree. While it has no functional problem, it's a obscure hack which
> means it is a code quality problem.
> 
> > Because there are no counters whose idx is larger than fixed
> > counters. However, it will have problem if new counter type is introduced
> > in generic code. For example, freerunning counters.
> >
> > Actually, the 'fixed counters' in the clinet IMC uncore is not
> > traditional fixed counter. They are freerunning counters, which don't
> > need the idx to indicate which counter is assigned. They also have same
> > bits wide. So it's OK to let them use the same idx. event_base is good
> 
> s/wide/width/
> 
> > enough to select the proper freerunning counter.
> 
> So why are they named fixed counters in the first place? If they are not
> fixed, but freerunning then please clean that up as well.
>

Sure, I will clean it up and make it part of the new free running infrastructure.
I will also modify all changelog according to your comments.

Thank you for the detailed review and your patience.
Kan

 
> I pointed out to you that this is crap. So please don't try to justify this
> crap. Just fix it up.
> 
> > There is no traditional fixed counter in clinet IMC uncore. Let them use
> > the same idx as fixed event for clinet IMC uncore events.
> 
> I have no idea what's traditional about counters, but that's a nit pick.
> 
> > The following patch will remove the special codes in generic
> > uncore_perf_event_update.
> 
> I told you more than once, that 'The ... patch', 'This patch' is not part
> of a proper changelog.
> 
> See Documentation/process/submitting-patches.rst:
> 
>     Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>     instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>     to do frotz", as if you are giving orders to the codebase to change
>     its behaviour.
> 
> along with the rest of the document.
> 
> Thanks,
> 
> 	tglx