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

Groups > linux.kernel > #1555850 > unrolled thread

[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

  [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-10 20:40 +0100
    Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for  MBA Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 15:20 +0100
      Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for  MBA Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-18 02:10 +0100
        Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for  MBA Thomas Gleixner <tglx@linutronix.de> - 2017-01-18 10:10 +0100
          Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for  MBA Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-23 20:50 +0100

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

Add the 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.
 - max_thrtl_by: the max throttle by values.

Throttle by can have a linear scale and non linear scale.  In linear
scale, if a throttle_by value is 40%, it means that the memory b/w is
throttled 'by' 40% or in other words a max of 60% b/w is allowed to
pass. In non-linear scale, the throttle_by values are in 2^n
granularity. The h/w does not guarantee a curve of actual throttle w.r.t
the configured values. But if a throttle_by value of x > y, then x is
guaranteed to throttle more b/w than y.
 - throttle granularity: Shows the throttle granularity of the throttle
 values that can be configured.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/include/asm/intel_rdt.h         |  8 ++-
 arch/x86/kernel/cpu/intel_rdt.c          |  2 +
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 84 ++++++++++++++++++++++++++++++--
 3 files changed, 89 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 49ae832..88725b6 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -5,7 +5,6 @@
 
 #include <linux/kernfs.h>
 #include <linux/jump_label.h>
-
 #include <asm/intel_rdt_common.h>
 
 #define IA32_L3_QOS_CFG		0xc81
@@ -77,6 +76,8 @@ struct rftype {
  * @no_ctrl:			Specifies max cache cbm or min mem b/w delay.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @info_files:		resctrl info files for the resource
+ * @infofiles_len:		Number of info files
  * @max_delay:		Max throttle delay
  * @delay_gran:		Throttle delay granularity
  * @delay_linear:		true if delay is in linear scale
@@ -98,6 +99,8 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			no_ctrl;
+	struct rftype		*info_files;
+	int			infofiles_len;
 	u32			max_delay;
 	u32			delay_gran;
 	u32			delay_linear;
@@ -137,6 +140,9 @@ struct msr_param {
 	int			high;
 };
 
+void rdt_get_cache_infofile(struct rdt_resource *r);
+void rdt_get_mbe_infofile(struct rdt_resource *r);
+
 extern struct mutex rdtgroup_mutex;
 
 extern struct rdt_resource rdt_resources_all[];
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 6736e1d..bdfbd1d 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -154,6 +154,7 @@ static void rdt_get_mem_config(struct rdt_resource *r)
 	if (r->delay_linear)
 		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
 
+	rdt_get_mbe_infofile(r);
 	r->capable = true;
 	r->enabled = true;
 }
@@ -168,6 +169,7 @@ static void rdt_get_cache_config(int idx, struct rdt_resource *r)
 	r->num_closid = edx.split.cos_max + 1;
 	r->cbm_len = eax.split.cbm_len + 1;
 	r->no_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
+	rdt_get_cache_infofile(r);
 	r->capable = true;
 	r->enabled = true;
 }
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 53f1917..9d9b7f4 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -517,9 +517,41 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 
 	return 0;
 }
+static int rdt_max_delay_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->max_delay);
+
+	return 0;
+}
+
+static int rdt_delay_gran_show(struct kernfs_open_file *of,
+			     struct seq_file *seq, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+
+	if (r->delay_linear)
+		seq_printf(seq, "%d\n", r->delay_gran);
+	else
+		seq_printf(seq, "power of 2\n");
+
+	return 0;
+}
+
+static int rdt_delay_linear_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->delay_linear);
+
+	return 0;
+}
 
 /* rdtgroup information files for one cache resource. */
-static struct rftype res_info_files[] = {
+static struct rftype res_cache_info_files[] = {
 	{
 		.name		= "num_closids",
 		.mode		= 0444,
@@ -540,11 +572,52 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 	},
 };
 
+/* rdtgroup information files for MBE. */
+static struct rftype res_mbe_info_files[] = {
+	{
+		.name		= "num_closids",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_num_closids_show,
+	},
+	{
+		.name		= "max_thrtl_by",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_max_delay_show,
+	},
+	{
+		.name		= "thrtl_by_gran",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_delay_gran_show,
+	},
+	{
+		.name		= "thrtl_by_linear",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_delay_linear_show,
+	},
+};
+
+void rdt_get_mbe_infofile(struct rdt_resource *r)
+{
+	r->info_files = &res_mbe_info_files[0];
+	r->infofiles_len = ARRAY_SIZE(res_mbe_info_files);
+}
+
+void rdt_get_cache_infofile(struct rdt_resource *r)
+{
+	r->info_files = &res_cache_info_files[0];
+	r->infofiles_len = ARRAY_SIZE(res_cache_info_files);
+}
+
 static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 {
 	struct kernfs_node *kn_subdir;
+	struct rftype *res_info_files;
 	struct rdt_resource *r;
-	int ret;
+	int ret, len;
 
 	/* create the directory */
 	kn_info = kernfs_create_dir(parent_kn, "info", parent_kn->mode, NULL);
@@ -563,8 +636,11 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 		ret = rdtgroup_kn_set_ugid(kn_subdir);
 		if (ret)
 			goto out_destroy;
-		ret = rdtgroup_add_files(kn_subdir, res_info_files,
-					 ARRAY_SIZE(res_info_files));
+
+		res_info_files = r->info_files;
+		len = r->infofiles_len;
+
+		ret = rdtgroup_add_files(kn_subdir, res_info_files, len);
 		if (ret)
 			goto out_destroy;
 		kernfs_activate(kn_subdir);
-- 
1.9.1

[toc] | [next] | [standalone]

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

On Tue, 10 Jan 2017, Vikas Shivappa wrote:

> Add the 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.
>  - max_thrtl_by: the max throttle by values.
> 
> Throttle by can have a linear scale and non linear scale.  In linear
> scale, if a throttle_by value is 40%, it means that the memory b/w is
> throttled 'by' 40% or in other words a max of 60% b/w is allowed to
> pass. In non-linear scale, the throttle_by values are in 2^n
> granularity. The h/w does not guarantee a curve of actual throttle w.r.t
> the configured values. But if a throttle_by value of x > y, then x is
> guaranteed to throttle more b/w than y.

This is ambigous because that is only correct when the effective values are
different. x=11 and y=12 with a granularity of 10 are resulting in the same
throttling.

> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -5,7 +5,6 @@
>  
>  #include <linux/kernfs.h>
>  #include <linux/jump_label.h>
> -
>  #include <asm/intel_rdt_common.h>

This white space is there on purpose to seperate linux and asm prefixed
includes. Stop making random white space changes.

> @@ -77,6 +76,8 @@ struct rftype {
>   * @no_ctrl:			Specifies max cache cbm or min mem b/w delay.
>   * @min_cbm_bits:		Minimum number of consecutive bits to be set
>   *				in a cache bit mask
> + * @info_files:		resctrl info files for the resource
> + * @infofiles_len:		Number of info files

This wants to be a seperate patch as it changes the way how the info files
are set up. The implementation for the MBA stuff is a follow up patch.

>   * @max_delay:		Max throttle delay
>   * @delay_gran:		Throttle delay granularity
>   * @delay_linear:		true if delay is in linear scale
> @@ -98,6 +99,8 @@ struct rdt_resource {
>  	int			cbm_len;
>  	int			min_cbm_bits;
>  	u32			no_ctrl;
> +	struct rftype		*info_files;
> +	int			infofiles_len;
>  	u32			max_delay;
>  	u32			delay_gran;
>  	u32			delay_linear;
> @@ -137,6 +140,9 @@ struct msr_param {
>  	int			high;
>  };
>  
> +void rdt_get_cache_infofile(struct rdt_resource *r);
> +void rdt_get_mbe_infofile(struct rdt_resource *r);
> +
>  extern struct mutex rdtgroup_mutex;
>  
>  extern struct rdt_resource rdt_resources_all[];
> diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
> index 6736e1d..bdfbd1d 100644
> --- a/arch/x86/kernel/cpu/intel_rdt.c
> +++ b/arch/x86/kernel/cpu/intel_rdt.c
> @@ -154,6 +154,7 @@ static void rdt_get_mem_config(struct rdt_resource *r)
>  	if (r->delay_linear)
>  		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
>  
> +	rdt_get_mbe_infofile(r);
>  	r->capable = true;
>  	r->enabled = true;
>  }
> @@ -168,6 +169,7 @@ static void rdt_get_cache_config(int idx, struct rdt_resource *r)
>  	r->num_closid = edx.split.cos_max + 1;
>  	r->cbm_len = eax.split.cbm_len + 1;
>  	r->no_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
> +	rdt_get_cache_infofile(r);
>  	r->capable = true;
>  	r->enabled = true;
>  }
> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> index 53f1917..9d9b7f4 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -517,9 +517,41 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
>  
>  	return 0;
>  }
> +static int rdt_max_delay_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->max_delay);
> +
> +	return 0;
> +}
> +
> +static int rdt_delay_gran_show(struct kernfs_open_file *of,
> +			     struct seq_file *seq, void *v)
> +{
> +	struct rdt_resource *r = of->kn->parent->priv;
> +
> +	if (r->delay_linear)
> +		seq_printf(seq, "%d\n", r->delay_gran);
> +	else
> +		seq_printf(seq, "power of 2\n");
> +
> +	return 0;
> +}
> +
> +static int rdt_delay_linear_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->delay_linear);
> +
> +	return 0;
> +}
>  
>  /* rdtgroup information files for one cache resource. */
> -static struct rftype res_info_files[] = {
> +static struct rftype res_cache_info_files[] = {
>  	{
>  		.name		= "num_closids",
>  		.mode		= 0444,
> @@ -540,11 +572,52 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
>  	},
>  };
>  
> +/* rdtgroup information files for MBE. */
> +static struct rftype res_mbe_info_files[] = {
> +	{
> +		.name		= "num_closids",
> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= rdt_num_closids_show,
> +	},
> +	{
> +		.name		= "max_thrtl_by",

You surely could not come up with a more cryptic file name, right? What's
so wrong with spelling out throttle? And the whole '_by' postfix here and
on the other files is pointless as well.

Thanks,

	tglx

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

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

On Mon, 16 Jan 2017, Thomas Gleixner wrote:

> On Tue, 10 Jan 2017, Vikas Shivappa wrote:
>
>> Add the 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.
>>  - max_thrtl_by: the max throttle by values.
>>
>> Throttle by can have a linear scale and non linear scale.  In linear
>> scale, if a throttle_by value is 40%, it means that the memory b/w is
>> throttled 'by' 40% or in other words a max of 60% b/w is allowed to
>> pass. In non-linear scale, the throttle_by values are in 2^n
>> granularity. The h/w does not guarantee a curve of actual throttle w.r.t
>> the configured values. But if a throttle_by value of x > y, then x is
>> guaranteed to throttle more b/w than y.
>
> This is ambigous because that is only correct when the effective values are
> different. x=11 and y=12 with a granularity of 10 are resulting in the same
> throttling.

The x and y are actual values written which i will spell out. The assumption is 
that only correct values are input though because we filterout the values which 
dont follow granularity, meaning return -EINVAL 
when someone tries to write 11 when granularity is 10. This was with the idea 
thats its easier for the user to understand whats actually written. Woudl that 
be reasonable or does it need a change ?
(Although the h/w does like you say , we can do a msr write for 11 etc ..)

>
>> --- a/arch/x86/include/asm/intel_rdt.h
>> +++ b/arch/x86/include/asm/intel_rdt.h
>> @@ -5,7 +5,6 @@
>>
>>  #include <linux/kernfs.h>
>>  #include <linux/jump_label.h>
>> -
>>  #include <asm/intel_rdt_common.h>
>
> This white space is there on purpose to seperate linux and asm prefixed
> includes. Stop making random white space changes.
>
>> @@ -77,6 +76,8 @@ struct rftype {
>>   * @no_ctrl:			Specifies max cache cbm or min mem b/w delay.
>>   * @min_cbm_bits:		Minimum number of consecutive bits to be set
>>   *				in a cache bit mask
>> + * @info_files:		resctrl info files for the resource
>> + * @infofiles_len:		Number of info files
>
> This wants to be a seperate patch as it changes the way how the info files
> are set up. The implementation for the MBA stuff is a follow up patch.

Ok , will split

>
>>   * @max_delay:		Max throttle delay
>>   * @delay_gran:		Throttle delay granularity
>>   * @delay_linear:		true if delay is in linear scale
>> @@ -98,6 +99,8 @@ struct rdt_resource {
>>  	int			cbm_len;
>>  	int			min_cbm_bits;
>>  	u32			no_ctrl;
>> +	struct rftype		*info_files;
>> +	int			infofiles_len;
>>  	u32			max_delay;
>>  	u32			delay_gran;
>>  	u32			delay_linear;
>> @@ -137,6 +140,9 @@ struct msr_param {
>>  	int			high;
>>  };
>>
>> +void rdt_get_cache_infofile(struct rdt_resource *r);
>> +void rdt_get_mbe_infofile(struct rdt_resource *r);
>> +
>>  extern struct mutex rdtgroup_mutex;
>>
>>  extern struct rdt_resource rdt_resources_all[];
>> diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
>> index 6736e1d..bdfbd1d 100644
>> --- a/arch/x86/kernel/cpu/intel_rdt.c
>> +++ b/arch/x86/kernel/cpu/intel_rdt.c
>> @@ -154,6 +154,7 @@ static void rdt_get_mem_config(struct rdt_resource *r)
>>  	if (r->delay_linear)
>>  		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
>>
>> +	rdt_get_mbe_infofile(r);
>>  	r->capable = true;
>>  	r->enabled = true;
>>  }
>> @@ -168,6 +169,7 @@ static void rdt_get_cache_config(int idx, struct rdt_resource *r)
>>  	r->num_closid = edx.split.cos_max + 1;
>>  	r->cbm_len = eax.split.cbm_len + 1;
>>  	r->no_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>> +	rdt_get_cache_infofile(r);
>>  	r->capable = true;
>>  	r->enabled = true;
>>  }
>> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
>> index 53f1917..9d9b7f4 100644
>> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
>> @@ -517,9 +517,41 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
>>
>>  	return 0;
>>  }
>> +static int rdt_max_delay_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->max_delay);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rdt_delay_gran_show(struct kernfs_open_file *of,
>> +			     struct seq_file *seq, void *v)
>> +{
>> +	struct rdt_resource *r = of->kn->parent->priv;
>> +
>> +	if (r->delay_linear)
>> +		seq_printf(seq, "%d\n", r->delay_gran);
>> +	else
>> +		seq_printf(seq, "power of 2\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int rdt_delay_linear_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->delay_linear);
>> +
>> +	return 0;
>> +}
>>
>>  /* rdtgroup information files for one cache resource. */
>> -static struct rftype res_info_files[] = {
>> +static struct rftype res_cache_info_files[] = {
>>  	{
>>  		.name		= "num_closids",
>>  		.mode		= 0444,
>> @@ -540,11 +572,52 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
>>  	},
>>  };
>>
>> +/* rdtgroup information files for MBE. */
>> +static struct rftype res_mbe_info_files[] = {
>> +	{
>> +		.name		= "num_closids",
>> +		.mode		= 0444,
>> +		.kf_ops		= &rdtgroup_kf_single_ops,
>> +		.seq_show	= rdt_num_closids_show,
>> +	},
>> +	{
>> +		.name		= "max_thrtl_by",
>
> You surely could not come up with a more cryptic file name, right? What's
> so wrong with spelling out throttle? And the whole '_by' postfix here and
> on the other files is pointless as well.

This is due to the issue i mention in reply to 1/8.. Can be changed to something 
better though. bw_restrict / bw_block (block is stopping..) ?

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>

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

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

On Tue, 17 Jan 2017, Shivappa Vikas wrote:
> On Mon, 16 Jan 2017, Thomas Gleixner wrote:
> 
> > On Tue, 10 Jan 2017, Vikas Shivappa wrote:
> > 
> > > Add the 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.
> > >  - max_thrtl_by: the max throttle by values.
> > > 
> > > Throttle by can have a linear scale and non linear scale.  In linear
> > > scale, if a throttle_by value is 40%, it means that the memory b/w is
> > > throttled 'by' 40% or in other words a max of 60% b/w is allowed to
> > > pass. In non-linear scale, the throttle_by values are in 2^n
> > > granularity. The h/w does not guarantee a curve of actual throttle w.r.t
> > > the configured values. But if a throttle_by value of x > y, then x is
> > > guaranteed to throttle more b/w than y.
> > 
> > This is ambigous because that is only correct when the effective values are
> > different. x=11 and y=12 with a granularity of 10 are resulting in the same
> > throttling.
> 
> The x and y are actual values written which i will spell out. The assumption
> is that only correct values are input though because we filterout the values
> which dont follow granularity, meaning return -EINVAL when someone tries to
> write 11 when granularity is 10. This was with the idea thats its easier for
> the user to understand whats actually written. Woudl that be reasonable or
> does it need a change ?
> (Although the h/w does like you say , we can do a msr write for 11 etc ..)


> > > +/* rdtgroup information files for MBE. */
> > > +static struct rftype res_mbe_info_files[] = {
> > > +	{
> > > +		.name		= "num_closids",
> > > +		.mode		= 0444,
> > > +		.kf_ops		= &rdtgroup_kf_single_ops,
> > > +		.seq_show	= rdt_num_closids_show,
> > > +	},
> > > +	{
> > > +		.name		= "max_thrtl_by",
> > 
> > You surely could not come up with a more cryptic file name, right? What's
> > so wrong with spelling out throttle? And the whole '_by' postfix here and
> > on the other files is pointless as well.
> 
> This is due to the issue i mention in reply to 1/8.. Can be changed to

Err no. max_thrtl_by has nothing to do with 1/8. Of course you want to
expose the hardware information.

What's wrong with spelling out throttle and remove the '_by':

       max_throttle

That is readable, understandable and matches the documentation in the SDM.

It's not that hard.

Thanks,

	tglx

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

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

On Wed, 18 Jan 2017, Thomas Gleixner wrote:

> On Tue, 17 Jan 2017, Shivappa Vikas wrote:
>> On Mon, 16 Jan 2017, Thomas Gleixner wrote:
>>
>>> On Tue, 10 Jan 2017, Vikas Shivappa wrote:
>>>
>>>> Add the 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.
>>>>  - max_thrtl_by: the max throttle by values.
>>>>
>>>> Throttle by can have a linear scale and non linear scale.  In linear
>>>> scale, if a throttle_by value is 40%, it means that the memory b/w is
>>>> throttled 'by' 40% or in other words a max of 60% b/w is allowed to
>>>> pass. In non-linear scale, the throttle_by values are in 2^n
>>>> granularity. The h/w does not guarantee a curve of actual throttle w.r.t
>>>> the configured values. But if a throttle_by value of x > y, then x is
>>>> guaranteed to throttle more b/w than y.
>>>
>>> This is ambigous because that is only correct when the effective values are
>>> different. x=11 and y=12 with a granularity of 10 are resulting in the same
>>> throttling.
>>
>> The x and y are actual values written which i will spell out. The assumption
>> is that only correct values are input though because we filterout the values
>> which dont follow granularity, meaning return -EINVAL when someone tries to
>> write 11 when granularity is 10. This was with the idea thats its easier for
>> the user to understand whats actually written. Woudl that be reasonable or
>> does it need a change ?
>> (Although the h/w does like you say , we can do a msr write for 11 etc ..)
>
>
>>>> +/* rdtgroup information files for MBE. */
>>>> +static struct rftype res_mbe_info_files[] = {
>>>> +	{
>>>> +		.name		= "num_closids",
>>>> +		.mode		= 0444,
>>>> +		.kf_ops		= &rdtgroup_kf_single_ops,
>>>> +		.seq_show	= rdt_num_closids_show,
>>>> +	},
>>>> +	{
>>>> +		.name		= "max_thrtl_by",
>>>
>>> You surely could not come up with a more cryptic file name, right? What's
>>> so wrong with spelling out throttle? And the whole '_by' postfix here and
>>> on the other files is pointless as well.
>>
>> This is due to the issue i mention in reply to 1/8.. Can be changed to
>
> Err no. max_thrtl_by has nothing to do with 1/8. Of course you want to
> expose the hardware information.
>
> What's wrong with spelling out throttle and remove the '_by':
>
>       max_throttle

Always was under the impression max_throttle by default means the max b/w thats 
allowed. (if max_throttle is 90, user would think 90% is whats the most allowed 
to pass..). But this is really the delay value which was implemented which 
means the % of bytes that are 'not' allowed to pass (so 90 means , that 90% is whats 
restricted and 10 is what is allowed).

Thanks,
Vikas

>
> That is readable, understandable and matches the documentation in the SDM.
>
> It's not that hard.
>
> Thanks,
>
> 	tglx
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web