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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 20 Oct 2017 15:04:48 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     Peter Zijlstra <peterz@...radead.org>,
        "x86@...nel.org" <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "acme@...nel.org" <acme@...nel.org>,
        "eranian@...gle.com" <eranian@...gle.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>
Subject: RE: [PATCH V2 1/4] perf/x86/intel/uncore: use same idx for clinet
 IMC uncore events

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