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

Groups > linux.kernel > #1590487 > unrolled thread

Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA

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 6/8] x86/intel_rdt/mba: Add info directory files for  MBA Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 17:30 +0100
    Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for  MBA Shivappa Vikas <vikas.shivappa@linux.intel.com> - 2017-03-11 00:30 +0100

#1590487 — Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

> Add files in info directory for MBA.
> The files in the info directory are as follows :
> - num_closids: max number of closids for MBA which represents the max
> class of service user can configure.
> - min_bw: the minimum memory bandwidth(b/w) values in percentage.
> 
> OS maps the b/w percentage values to memory b/w throttle delay values
> and configures them via MSR interface by writing to the QOS_MSRs. These
> delay values can have a linear or nonlinear scale.
> 
> - bw_gran: The memory b/w granularity that can be configured.
> For ex: If the granularity is 10% and min_bw is 10, valid bandwidth
> values are 10,20,30...

This is unreadable. It's possible to structure ASCII text cleanly.

x86/rdt: Add info directory files for MBA

 The info directory for MBA contains the following files:

 num_closids

	The maximum number of class of service slots available for MBA

 min_bandwidth
 
	The minimum memory bandwidth percentage value

 bandwidth_gran

	The granularity of the bandwidth control for the particular
 	hardware in percent. The available bandwidth control steps are:
 	min_bw + N * bw_gran Intermediate values are rounded to the next
 	control step available on the hardware.

 delay_linear

	If set, the control registers take a linear percentage based value
	between min_bandwidth and 100 percent.

	If not set, the control registers take a power of 2 based value
	which is mapped by the kernel to percentage based values.

	This file is of pure informational nature and has no influence on
	the values which are written to the schemata files. These are
	always percentage based.

Note, that this uses the actual file names and not some random
abbreviations thereof. It also documents delay_linear and gets rid of the
implementation details of QOS_MSRs. They are irrelevant here. 

And exactly this information wants to go into Documentation/... preferably
in exactly this patch and not in a disconnected one which describes stuff
differently for whatever reasons.
  
> +static int rdt_min_bw_show(struct kernfs_open_file *of,
> +			     struct seq_file *seq, void *v)
> +{
> +	struct rdt_resource *r = of->kn->parent->priv;
> +
> +	seq_printf(seq, "%d\n", r->min_bw);
> +

Can you please get rid of these pointless extra new lines before the
'return 0;' ? They are just eating screen estate and do not make the code
more readable.

> +/* rdtgroup information files for MBE. */

What is MBE?

> +static struct rftype res_mbe_info_files[] = {

Randomizing names make the code more secure or what are you trying to
achieve?

> +void rdt_get_mba_infofile(struct rdt_resource *r)
> +{
> +	r->info_files = &res_mbe_info_files[0];

See other mail.

Thanks,

	tglx

[toc] | [next] | [standalone]

#1598173

On Wed, 1 Mar 2017, Thomas Gleixner wrote:

> On Fri, 17 Feb 2017, Vikas Shivappa wrote:
>
>> Add files in info directory for MBA.
>> The files in the info directory are as follows :
>> - num_closids: max number of closids for MBA which represents the max
>> class of service user can configure.
>> - min_bw: the minimum memory bandwidth(b/w) values in percentage.
>>
>> OS maps the b/w percentage values to memory b/w throttle delay values
>> and configures them via MSR interface by writing to the QOS_MSRs. These
>> delay values can have a linear or nonlinear scale.
>>
>> - bw_gran: The memory b/w granularity that can be configured.
>> For ex: If the granularity is 10% and min_bw is 10, valid bandwidth
>> values are 10,20,30...
>
> This is unreadable. It's possible to structure ASCII text cleanly.
>
> x86/rdt: Add info directory files for MBA
>
> The info directory for MBA contains the following files:
>
> num_closids
>
> 	The maximum number of class of service slots available for MBA
>
> min_bandwidth
>
> 	The minimum memory bandwidth percentage value
>
> bandwidth_gran
>
> 	The granularity of the bandwidth control for the particular
> 	hardware in percent. The available bandwidth control steps are:
> 	min_bw + N * bw_gran Intermediate values are rounded to the next

Is this next or pervious? Meaning when 12 is requested on a 10 granularity , we 
give 10 ?

> 	control step available on the hardware.
>
> delay_linear
>
> 	If set, the control registers take a linear percentage based value
> 	between min_bandwidth and 100 percent.
>
> 	If not set, the control registers take a power of 2 based value
> 	which is mapped by the kernel to percentage based values.
>
> 	This file is of pure informational nature and has no influence on
> 	the values which are written to the schemata files. These are
> 	always percentage based.

Will update the changelogs

>
> Note, that this uses the actual file names and not some random
> abbreviations thereof. It also documents delay_linear and gets rid of the
> implementation details of QOS_MSRs. They are irrelevant here.
>
> And exactly this information wants to go into Documentation/... preferably
> in exactly this patch and not in a disconnected one which describes stuff
> differently for whatever reasons.
>
>> +static int rdt_min_bw_show(struct kernfs_open_file *of,
>> +			     struct seq_file *seq, void *v)
>> +{
>> +	struct rdt_resource *r = of->kn->parent->priv;
>> +
>> +	seq_printf(seq, "%d\n", r->min_bw);
>> +
>
> Can you please get rid of these pointless extra new lines before the
> 'return 0;' ? They are just eating screen estate and do not make the code
> more readable.

All the rest of the show functions have that line before return like the 
rdt_min_cbm_bits_show etc.  I have tried to 
always keep a line before return like the old cqm/rapl - if thats ok

>
>> +/* rdtgroup information files for MBE. */
>
> What is MBE?
>
>> +static struct rftype res_mbe_info_files[] = {
>
> Randomizing names make the code more secure or what are you trying to
> achieve?
>
>> +void rdt_get_mba_infofile(struct rdt_resource *r)
>> +{
>> +	r->info_files = &res_mbe_info_files[0];
>
> See other mail.
>

Will fix the MBE to MBA and the pointer init.

Thanks,
Vikas

> Thanks,
>
> 	tglx
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web