Date: Fri, 10 Mar 2017 15:26:34 -0800 (PST)
From: Shivappa Vikas <vikas.shivappa@linux.intel.com>
To: Thomas Gleixner <tglx@linutronix.de>
cc: Vikas Shivappa <vikas.shivappa@linux.intel.com>,
        vikas.shivappa@intel.com, linux-kernel@vger.kernel.org, x86@kernel.org,
        hpa@zytor.com, mingo@kernel.org, peterz@infradead.org,
        ravi.v.shankar@intel.com, tony.luck@intel.com, fenghua.yu@intel.com,
        andi.kleen@intel.com
Subject: Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for
 MBA
In-Reply-To: <alpine.DEB.2.20.1703011634560.4005@nanos>
Message-ID: <alpine.DEB.2.10.1703101524110.5245@vshiva-Udesk>
References: <1487361535-9727-1-git-send-email-vikas.shivappa@linux.intel.com> <1487361535-9727-7-git-send-email-vikas.shivappa@linux.intel.com> <alpine.DEB.2.20.1703011634560.4005@nanos>
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: 3261
Lines: 110


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
>