lkml.org 
[lkml]   [2017]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V2 1/4] perf/x86/intel/uncore: use same idx for clinet IMC uncore events
Date
> > 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

\
 
 \ /
  Last update: 2017-10-22 17:42    [W:0.052 / U:0.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site