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

[Thomas Gleixner]

On Thu, 19 Oct 2017, kan.liang@xxxxxxxxx wrote:
> The clinet IMC uncore is the only one who claims two 'fixed counters'.

clinet? 

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

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