linux-kernel - Re: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth Monitoring (MBM) PMU

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20150911211117.GB2537@codeblueprint.co.uk>
Date:	Fri, 11 Sep 2015 22:11:17 +0100
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	Kanaka Juvva <kanaka.d.juvva@...el.com>
Cc:	Matt Fleming <matt.fleming@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Kanaka Juvva <kanaka.d.juvva@...ux.intel.com>,
	"Williamson, Glenn P" <glenn.p.williamson@...el.com>,
	"Auld, Will" <will.auld@...el.com>,
	Andi Kleen <andi@...stfloor.org>,
	LKML <linux-kernel@...r.kernel.org>,
	"Luck, Tony" <tony.luck@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Tejun Heo <tj@...nel.org>, "x86@...nel.org" <x86@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Shivappa, Vikas" <vikas.shivappa@...el.com>
Subject: Re: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth Monitoring
 (MBM) PMU

On Thu, 10 Sep, at 02:18:49PM, Kanaka Juvva wrote:
> > > 
> > > > >         } else {
> > > > >                 mbm_current = &mbm_total[vrmid];
> > > > >                 eventid     = QOS_MBM_TOTAL_EVENT_ID;
> > > > >         }
> > > > >         rmid = tmp32;
> > > > 
> > > > Why did you assign rmid to vrmid if you reassign it before it was used?
> > > > 
> > > > 
> > > 
> > > For MSR writes we use rmid value and for mbm_* arrary we use vrmid which is actual
> > > index.
> > 
> > What I'm saying is that the assignment rmid = vrmid looks unnecessary in
> > this piece of code.
> > 
> 
> From my previous review:
> 
>   "This is completely backwards.
>  
>         tmp32 = rmid;  
>         rmid = vrmid;
>         do_stuff(rmid);
>         rmid = tmp32;
>         do_other_stuff(rmid);
> 
>    Why can't you use vrmid for do_stuff() and leave rmid alone? Just
>    because it would make the code simpler to read?"
> 
> I have included Thomas comment inline above.
> 
> and also I meant the following logic:
> 
> writemsr(..,rmid,...)
> mbm_*[vrmid]  
> 
> So new patch will use this logic.
 
OK, let's pull the code in and discuss this with some context,

	u64 val, currentmsr, currentbw, diff_time, cma, bytes, index;                                                                                                                      
        bool overflow = false, first = false;
        ktime_t cur_time;
        u32 tmp32 = rmid, eventid;
        struct sample *mbm_current;
        u32 vrmid = rmid_2_index(rmid);
 
        rmid = vrmid;			<--------- This looks wrong
        cur_time = ktime_get();
        if (read_mbm_local) {
                mbm_current = &mbm_local[vrmid];
                eventid     =  QOS_MBM_LOCAL_EVENT_ID_HW;
                wrmsr(MSR_IA32_QM_EVTSEL, QOS_MBM_LOCAL_EVENT_ID_HW, rmid);	<---- Unneccesary because..
        } else {
                mbm_current = &mbm_total[vrmid];
                eventid     = QOS_MBM_TOTAL_EVENT_ID;
        }
        rmid = tmp32;
        wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);	<----- ... you write here
 
So you don't actually use 'rmid' within that if/else block. You can
probably get away with deleting 'tmp32' now that you've refactored
things.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/