Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1352111 > unrolled thread

Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event management

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event management Peter Zijlstra <peterz@infradead.org> - 2016-03-08 00:10 +0100
    RE: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event  management "Luck, Tony" <tony.luck@intel.com> - 2016-03-08 00:30 +0100
      Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event management Peter Zijlstra <peterz@infradead.org> - 2016-03-08 09:50 +0100
        Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event  management Vikas Shivappa <vikas.shivappa@intel.com> - 2016-03-10 23:50 +0100
    Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event  management Vikas Shivappa <vikas.shivappa@intel.com> - 2016-03-11 00:00 +0100

#1352111 — Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event management

On Tue, Mar 01, 2016 at 03:48:26PM -0800, Vikas Shivappa wrote:

> Lot of the scheduling code was taken out from Tony's patch and a 3-4
> lines of change were added in the intel_cqm_event_read. Since the timer
> is no more added on every context switch this change was made.

It this here to confuse people or is there some actual information in
it?

> +/*
> + * MBM Counter is 24bits wide. MBM_CNTR_MAX defines max counter
> + * value
> + */
> +#define MBM_CNTR_MAX		0xffffff

#define MBM_CNTR_WIDTH	24
#define MBM_CNTR_MAX	((1U << MBM_CNTR_WIDTH) - 1)


>  #define QOS_L3_OCCUP_EVENT_ID	(1 << 0)
> +/*
> + * MBM Event IDs as defined in SDM section 17.15.5
> + * Event IDs are used to program EVTSEL MSRs before reading mbm event counters
> + */
> +enum mbm_evt_type {
> +	QOS_MBM_TOTAL_EVENT_ID = 0x02,
> +	QOS_MBM_LOCAL_EVENT_ID,
> +	QOS_MBM_TOTAL_BW_EVENT_ID,
> +	QOS_MBM_LOCAL_BW_EVENT_ID,
> +};

QOS_L3_*_EVENT_ID is a define, these are an enum. Rather inconsistent.

>  struct rmid_read {
>  	u32 rmid;

Hole, you could've filled with the enum (which ends up being an int I
think).

>  	atomic64_t value;
> +	enum mbm_evt_type evt_type;
>  };

> +static bool is_mbm_event(int e)

You had an enum type, you might as well use it.

> +{
> +	return (e >= QOS_MBM_TOTAL_EVENT_ID && e <= QOS_MBM_LOCAL_BW_EVENT_ID);
> +}
>  

> +static struct sample *update_sample(unsigned int rmid,
> +				    enum mbm_evt_type evt_type, int first)
> +{
> +	ktime_t cur_time;
> +	struct sample *mbm_current;
> +	u32 vrmid = rmid_2_index(rmid);
> +	u64 val, bytes, diff_time;
> +	u32 eventid;
> +
> +	if (evt_type & QOS_MBM_LOCAL_EVENT_MASK) {
> +		mbm_current = &mbm_local[vrmid];
> +		eventid     =  QOS_MBM_LOCAL_EVENT_ID;
> +	} else {
> +		mbm_current = &mbm_total[vrmid];
> +		eventid     = QOS_MBM_TOTAL_EVENT_ID;
> +	}
> +
> +	cur_time = ktime_get();
> +	wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);
> +	rdmsrl(MSR_IA32_QM_CTR, val);
> +	if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> +		return mbm_current;

> +	val &= MBM_CNTR_MAX;

> +	if (val < mbm_current->prev_msr)
> +		bytes = MBM_CNTR_MAX - mbm_current->prev_msr + val + 1;
> +	else
> +		bytes = val - mbm_current->prev_msr;

Would not something like:

	shift = 64 - MBM_CNTR_WIDTH;

	bytes = (val << shift) - (prev << shift);
	bytes >>= shift;

be less obtuse? (and consistent with how every other perf update
function does it).

What guarantee is there we didn't wrap multiple times? Doesn't that
deserve a comment?

> +	bytes *= cqm_l3_scale;
> +
> +	mbm_current->total_bytes += bytes;
> +	mbm_current->interval_bytes += bytes;
> +	mbm_current->prev_msr = val;
> +	diff_time = ktime_ms_delta(cur_time, mbm_current->interval_start);

Here we do a / 1e6

> +
> +	/*
> +	 * The b/w measured is really the most recent/current b/w.
> +	 * We wait till enough time has passed to avoid
> +	 * arthmetic rounding problems.Having it at >=100ms,
> +	 * such errors would be <=1%.
> +	 */
> +	if (diff_time > 100) {

This could well be > 100e6 instead, avoiding the above division most of
the time.

> +		bytes = mbm_current->interval_bytes * MSEC_PER_SEC;
> +		do_div(bytes, diff_time);
> +		mbm_current->bandwidth = bytes;
> +		mbm_current->interval_bytes = 0;
> +		mbm_current->interval_start = cur_time;
> +	}
> +
> +	return mbm_current;
> +}

How does the above time tracking deal with the event not actually having
been scheduled the whole time?


> +static void init_mbm_sample(u32 rmid, enum mbm_evt_type evt_type)
> +{
> +	struct rmid_read rr = {
> +		.value = ATOMIC64_INIT(0),
> +	};
> +
> +	rr.rmid = rmid;
> +	rr.evt_type = evt_type;

That's just sad.. put those two in the struct init as well.

> +	/* on each socket, init sample */
> +	on_each_cpu_mask(&cqm_cpumask, __intel_mbm_event_init, &rr, 1);
> +}

[toc] | [next] | [standalone]

#1352180 — RE: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event management

>> +		bytes = mbm_current->interval_bytes * MSEC_PER_SEC;
>> +		do_div(bytes, diff_time);
>> +		mbm_current->bandwidth = bytes;
>> +		mbm_current->interval_bytes = 0;
>> +		mbm_current->interval_start = cur_time;
>> +	}
>>> +
>> +	return mbm_current;
>> +}
>
> How does the above time tracking deal with the event not actually having
> been scheduled the whole time?

That's been the topic of a few philosophical debates ... what exactly are
we trying to say when we report that a process has a "memory bandwidth"
of, say, 1523 MBytes/s?  We need to know both the amount of data moved
and to pick an interval to measure and divide by. Does it make a difference
whether the process voluntarily gave up the cpu for some part of the interval
(by blocking on I/O)? Or did the scheduler time-slice it out to run other jobs?

The above code gives the average bandwidth across the last interval
(with a minimum interval size of 100ms to avoid craziness with rounding
errors on exceptionally tiny intervals). Some folks apparently want to get
a "rate" directly from perf.  I think many folks will find the "bytes" counters
more helpful (where they control the sample interval with '-I" flag to perf
utility).

-Tony

[toc] | [prev] | [next] | [standalone]

#1352752

On Mon, Mar 07, 2016 at 11:27:26PM +0000, Luck, Tony wrote:
> >> +		bytes = mbm_current->interval_bytes * MSEC_PER_SEC;
> >> +		do_div(bytes, diff_time);
> >> +		mbm_current->bandwidth = bytes;
> >> +		mbm_current->interval_bytes = 0;
> >> +		mbm_current->interval_start = cur_time;
> >> +	}
> >>> +
> >> +	return mbm_current;
> >> +}
> >
> > How does the above time tracking deal with the event not actually having
> > been scheduled the whole time?
> 
> That's been the topic of a few philosophical debates ... what exactly are
> we trying to say when we report that a process has a "memory bandwidth"
> of, say, 1523 MBytes/s?  We need to know both the amount of data moved
> and to pick an interval to measure and divide by. Does it make a difference
> whether the process voluntarily gave up the cpu for some part of the interval
> (by blocking on I/O)? Or did the scheduler time-slice it out to run other jobs?
> 
> The above code gives the average bandwidth across the last interval
> (with a minimum interval size of 100ms to avoid craziness with rounding
> errors on exceptionally tiny intervals). Some folks apparently want to get
> a "rate" directly from perf.  I think many folks will find the "bytes" counters
> more helpful (where they control the sample interval with '-I" flag to perf
> utility).

So why didn't any of that make it into the Changelog? This is very much
different from any other perf driver, at the very least this debate
should have been mentioned and the choice defended.

Also, why are we doing the time tracking and divisions at all? Can't we
simply report the number of bytes transferred and let userspace sort out
the rest?

Userspace is provided the time the event was enabled, the time the event
was running and it can fairly trivially obtain walltime if it so desires
and then it can compute whatever definition of bandwidth it wants to
use.

[toc] | [prev] | [next] | [standalone]

#1355436 — Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event management

On Tue, 8 Mar 2016, Peter Zijlstra wrote:

> On Mon, Mar 07, 2016 at 11:27:26PM +0000, Luck, Tony wrote:
>>>> +		bytes = mbm_current->interval_bytes * MSEC_PER_SEC;
>>>> +		do_div(bytes, diff_time);
>>>> +		mbm_current->bandwidth = bytes;
>>>> +		mbm_current->interval_bytes = 0;
>>>> +		mbm_current->interval_start = cur_time;
>>>> +	}
>>>>> +
>>>> +	return mbm_current;
>>>> +}
>>>
>>> How does the above time tracking deal with the event not actually having
>>> been scheduled the whole time?
>>
>> That's been the topic of a few philosophical debates ... what exactly are
>> we trying to say when we report that a process has a "memory bandwidth"
>> of, say, 1523 MBytes/s?  We need to know both the amount of data moved
>> and to pick an interval to measure and divide by. Does it make a difference
>> whether the process voluntarily gave up the cpu for some part of the interval
>> (by blocking on I/O)? Or did the scheduler time-slice it out to run other jobs?
>>
>> The above code gives the average bandwidth across the last interval
>> (with a minimum interval size of 100ms to avoid craziness with rounding
>> errors on exceptionally tiny intervals). Some folks apparently want to get
>> a "rate" directly from perf.  I think many folks will find the "bytes" counters
>> more helpful (where they control the sample interval with '-I" flag to perf
>> utility).
>
> So why didn't any of that make it into the Changelog? This is very much
> different from any other perf driver, at the very least this debate
> should have been mentioned and the choice defended.
>
> Also, why are we doing the time tracking and divisions at all? Can't we
> simply report the number of bytes transferred and let userspace sort out
> the rest?
>
> Userspace is provided the time the event was enabled, the time the event
> was running and it can fairly trivially obtain walltime if it so desires
> and then it can compute whatever definition of bandwidth it wants to
> use.


We had discussions on removing the bw event. Discussed this with Tony and will 
update the patch by removing the bw events.. So this code will be removed.

thanks,
Vikas

>
>
>

[toc] | [prev] | [next] | [standalone]

#1355442 — Re: [PATCH 4/6] x86/mbm: Memory bandwidth monitoring event management

On Mon, 7 Mar 2016, Peter Zijlstra wrote:

> On Tue, Mar 01, 2016 at 03:48:26PM -0800, Vikas Shivappa wrote:
>
>> Lot of the scheduling code was taken out from Tony's patch and a 3-4
>> lines of change were added in the intel_cqm_event_read. Since the timer
>> is no more added on every context switch this change was made.
>
> It this here to confuse people or is there some actual information in
> it?

Will remove the comment.

>
>> +/*
>> + * MBM Counter is 24bits wide. MBM_CNTR_MAX defines max counter
>> + * value
>> + */
>> +#define MBM_CNTR_MAX		0xffffff
>
> #define MBM_CNTR_WIDTH	24
> #define MBM_CNTR_MAX	((1U << MBM_CNTR_WIDTH) - 1)
>
>
Will fix

>>  #define QOS_L3_OCCUP_EVENT_ID	(1 << 0)
>> +/*
>> + * MBM Event IDs as defined in SDM section 17.15.5
>> + * Event IDs are used to program EVTSEL MSRs before reading mbm event counters
>> + */
>> +enum mbm_evt_type {
>> +	QOS_MBM_TOTAL_EVENT_ID = 0x02,
>> +	QOS_MBM_LOCAL_EVENT_ID,
>> +	QOS_MBM_TOTAL_BW_EVENT_ID,
>> +	QOS_MBM_LOCAL_BW_EVENT_ID,
>> +};
>
> QOS_L3_*_EVENT_ID is a define, these are an enum. Rather inconsistent.
>

Will be changing all of them to #define . and we are also removing the bw 
events..

>>  struct rmid_read {
>>  	u32 rmid;
>
> Hole, you could've filled with the enum (which ends up being an int I
> think).
>
>>  	atomic64_t value;
>> +	enum mbm_evt_type evt_type;
>>  };
>
>> +static bool is_mbm_event(int e)
>
> You had an enum type, you might as well use it.

the enum will be gone..

>
>> +{
>> +	return (e >= QOS_MBM_TOTAL_EVENT_ID && e <= QOS_MBM_LOCAL_BW_EVENT_ID);
>> +}
>>
>
>> +static struct sample *update_sample(unsigned int rmid,
>> +				    enum mbm_evt_type evt_type, int first)
>> +{
>> +	ktime_t cur_time;
>> +	struct sample *mbm_current;
>> +	u32 vrmid = rmid_2_index(rmid);
>> +	u64 val, bytes, diff_time;
>> +	u32 eventid;
>> +
>> +	if (evt_type & QOS_MBM_LOCAL_EVENT_MASK) {
>> +		mbm_current = &mbm_local[vrmid];
>> +		eventid     =  QOS_MBM_LOCAL_EVENT_ID;
>> +	} else {
>> +		mbm_current = &mbm_total[vrmid];
>> +		eventid     = QOS_MBM_TOTAL_EVENT_ID;
>> +	}
>> +
>> +	cur_time = ktime_get();
>> +	wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);
>> +	rdmsrl(MSR_IA32_QM_CTR, val);
>> +	if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
>> +		return mbm_current;
>
>> +	val &= MBM_CNTR_MAX;
>
>> +	if (val < mbm_current->prev_msr)
>> +		bytes = MBM_CNTR_MAX - mbm_current->prev_msr + val + 1;
>> +	else
>> +		bytes = val - mbm_current->prev_msr;
>
> Would not something like:
>
> 	shift = 64 - MBM_CNTR_WIDTH;
>
> 	bytes = (val << shift) - (prev << shift);
> 	bytes >>= shift;
>
> be less obtuse? (and consistent with how every other perf update
> function does it).

Will fix.

>
> What guarantee is there we didn't wrap multiple times? Doesn't that
> deserve a comment?


this is taken care of in the next patch 0006. I have put a comment there that 
h/w guarentees that overflow wont happen with in 1s at the definition of the 
timers, but can add an other comment here in the patch 0006

>
>> +	bytes *= cqm_l3_scale;
>> +
>> +	mbm_current->total_bytes += bytes;
>> +	mbm_current->interval_bytes += bytes;
>> +	mbm_current->prev_msr = val;
>> +	diff_time = ktime_ms_delta(cur_time, mbm_current->interval_start);
>
> Here we do a / 1e6
>
>> +
>> +	/*
>> +	 * The b/w measured is really the most recent/current b/w.
>> +	 * We wait till enough time has passed to avoid
>> +	 * arthmetic rounding problems.Having it at >=100ms,
>> +	 * such errors would be <=1%.
>> +	 */
>> +	if (diff_time > 100) {
>
> This could well be > 100e6 instead, avoiding the above division most of
> the time.
>
>> +		bytes = mbm_current->interval_bytes * MSEC_PER_SEC;
>> +		do_div(bytes, diff_time);
>> +		mbm_current->bandwidth = bytes;
>> +		mbm_current->interval_bytes = 0;
>> +		mbm_current->interval_start = cur_time;
>> +	}
>> +
>> +	return mbm_current;
>> +}
>
> How does the above time tracking deal with the event not actually having
> been scheduled the whole time?

Will be removing the bw events - so should address all three comments above.

>
>
>> +static void init_mbm_sample(u32 rmid, enum mbm_evt_type evt_type)
>> +{
>> +	struct rmid_read rr = {
>> +		.value = ATOMIC64_INIT(0),
>> +	};
>> +
>> +	rr.rmid = rmid;
>> +	rr.evt_type = evt_type;
>
> That's just sad.. put those two in the struct init as well.

Will fix

thanks,
vikas
>
>> +	/* on each socket, init sample */
>> +	on_each_cpu_mask(&cqm_cpumask, __intel_mbm_event_init, &rr, 1);
>> +}
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web