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

[Thomas Gleixner]

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