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