Date: Mon, 25 Apr 2016 14:12:09 -0700 (PDT)
From: Vikas Shivappa <vikas.shivappa@intel.com>
To: Peter Zijlstra <peterz@infradead.org>
cc: Vikas Shivappa <vikas.shivappa@intel.com>,
        Vikas Shivappa <vikas.shivappa@linux.intel.com>, tony.luck@intel.com,
        ravi.v.shankar@intel.com, fenghua.yu@intel.com, x86@kernel.org,
        linux-kernel@vger.kernel.org, hpa@zytor.com, tglx@linutronix.de,
        mingo@kernel.org, h.peter.anvin@intel.com
Subject: Re: [PATCH 2/4] perf/x86/mbm: Store bytes counted for mbm during
 recycle
In-Reply-To: <20160425200222.GM3448@twins.programming.kicks-ass.net>
Message-ID: <alpine.DEB.2.10.1604251359040.18257@vshiva-Udesk>
References: <1461371241-4258-1-git-send-email-vikas.shivappa@linux.intel.com> <1461371241-4258-3-git-send-email-vikas.shivappa@linux.intel.com> <20160425091311.GE3430@twins.programming.kicks-ass.net> <alpine.DEB.2.10.1604251051120.18257@vshiva-Udesk>
 <20160425200222.GM3448@twins.programming.kicks-ass.net>
User-Agent: Alpine 2.10 (DEB 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2505
Lines: 80


On Mon, 25 Apr 2016, Peter Zijlstra wrote:

> On Mon, Apr 25, 2016 at 11:04:38AM -0700, Vikas Shivappa wrote:
>>> This is a 'creative' solution; why don't you do the normal thing, which
>>> is:
>>>
>>> start:
>>> 	prev_count = read_hw_counter();
>>>
>>> read:
>>> 	do {
>>> 		prev = prev_count;
>>> 		cur_val = read_hw_counter();
>>> 		delta = cur_val - prev;
>>> 	} while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
>>> 	count += delta;
>>
>>
>> I may need to update the comment.
>>
>> rc_count stores the total bytes for RMIDs that were used for this event
>> except for the count of current RMID.
>
> Yeah, I got that, eventually.
>
>> Say an event used RMID(1) .. RMID(k) from init to read and it had RMID(k)
>> when read was called, the rc_count stores the values read from RMID1 ..
>> RMID(k-1).
>>
>> For MBM the patch is trying to do:
>> count
>> = total_bytes of RMID(1) + ... +total_bytes of RMID(k-1) + total_bytes of
>> RMID(k))
>> = rc_count + total_bytes of RMID(k).
>
> How is the regular counting scheme as outlined above not dealing with
> this properly?
>
>

By regular if you mean the current upstream code
local64_set(&event->count, atomic64_read(&rr.value));

then note that the rr.value is just the current RMIDs total_bytes, then we loose 
the old values. So if RMID(1) counted 100MB , then RMID(2) counted 10MB and 
there was a read(which is actually count call for cqm) call after RMID(2) then 
it returns 10MB and not 100MB which is the real total_bytes..

if you mean the below -

>
> start:
>       prev_count = read_hw_counter();

I am assuming this means we keep the prev_count when event is initialized. This 
is done in the mbm_init which calls update_sample with first parameter set to 
true..


>
> read:
>       do {
>               prev = prev_count;
>               cur_val = read_hw_counter();
>               delta = cur_val - prev;
>       } while (local_cmpxchg(&prev_count, prev, cur_val) != prev);
>       count += delta;

the update_sample does the work to compute the delta and add the delta to 
total_bytes..  it has all the code except for the while loop.

So we miss the counter values of RMIDs which may 
be used by somebody else now.. they are stored during recycling just 
before we loose the RMID.
If you are tyring to count multiple RMIDs(that were used by the event) with the 
while loop(?) thats something we do but its done in the xchng when we loose the 
RMID.. as those counts are probably not there in those respective RMIDs anymore.