From: Shivappa Vikas <vikas.shivappa@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,
h.peter.anvin@intel.com
Subject: Re: [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for MBA
Date: Tue, 17 Jan 2017 17:03:51 -0800 (PST) [thread overview]
Message-ID: <alpine.DEB.2.10.1701171703140.15892@vshiva-Udesk> (raw)
In-Reply-To: <alpine.DEB.2.20.1701161655050.3877@nanos>
On Mon, 16 Jan 2017, Thomas Gleixner wrote:
> On Tue, 10 Jan 2017, Vikas Shivappa wrote:
>> + * @display_str: Format string to show schemata
>> + * @validate: API to validate the ctrl values.
>> * @info_files: resctrl info files for the resource
>> * @infofiles_len: Number of info files
>> * @max_delay: Max throttle delay
>> @@ -99,6 +101,9 @@ struct rdt_resource {
>> int cbm_len;
>> int min_cbm_bits;
>> u32 no_ctrl;
>> + char *display_str;
>> + int (*validate) (char *buf, unsigned long *data,
>> + struct rdt_resource *r);
>
> Again this display and validation change wants to be seperate from the
> bandwidth stuff.
>
> It's not rocket science to split patches into preparatory and
> implementation parts.
Will split.
>
>> + r->display_str = kstrdup("%d=%d", GFP_KERNEL);
>> + if (!r->display_str)
>> + return -ENOMEM;
>
> And the point of this allocation is? To consume extra memory for a constant
> string which is in const data anyway.
>
> r->display_str = "%d=%d";
Ok will fix to const string.
>
> does not need allcotion and consumes exactly the same amount of const data
> as the above. Oh well...
>
>> -static inline bool get_rdt_resources(void)
>> +static inline int get_rdt_resources(void)
>> {
>
> And the point of this change is? Lots of churn to return the same -ENODEV
> value at the call site. So why are you trying to return other values
> instead of the simple boolean success/fail decision?
>
>> /*
>> + * Check whether MBE 'throttle by' value is correct.
>> + * As per the SDM, when the scale is linear the
>> + * throttle_by granularity is '100 - max_thrtl_by'
>> + * and when its non-linear it is 'power of 2'.
>
> That's wrong. We really want to let the user set a bandwidth percentage
> value from 0 - 100 %. And then adjust it to the proper value which the
> hardware can provide. So the user value is independent from granularity,
> linear and the max throttling allowed.
>
>> /*
>> - * Read one cache bit mask (hex). Check that it is valid for the current
>> - * resource type.
>> + * Read the user RDT control value into tempory buffer:
>> + * Cache bit mask (hex) or Memory b/w throttle (decimal).
>> + * Check that it is valid for the current resource type.
>> */
>> -static int parse_cbm(char *buf, struct rdt_resource *r)
>> +static int parse_ctrls(char *buf, struct rdt_resource *r)
>> {
>> unsigned long data;
>> - int ret;
>> + int ret = 0;
>
> What's the purpose of initializing ret to 0 if the next action is assigning
> ret the return value of the validate function?
Will fix.
Thanks,
Vikas
>
>> - ret = kstrtoul(buf, 16, &data);
>> - if (ret)
>> - return ret;
>> - if (!cbm_validate(data, r))
>> - return -EINVAL;
>> + ret = r->validate(buf, &data, r);
>> r->tmp_ctrl[r->num_tmp_ctrl++] = data;
>>
>> - return 0;
>> + return ret;
>> }
>
> Thanks,
>
> tglx
>
next prev parent reply other threads:[~2017-01-18 1:03 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-10 19:33 [PATCH 0/8 V1] x86/intel_rdt: Memory b/w Allocation support Vikas Shivappa
2017-01-10 19:33 ` [PATCH 1/8] Documentation, x86: Documentation for Intel Mem b/w allocation user interface Vikas Shivappa
2017-01-16 13:43 ` Thomas Gleixner
2017-01-18 0:51 ` Shivappa Vikas
2017-01-18 9:01 ` Thomas Gleixner
2017-01-23 18:57 ` Shivappa Vikas
2017-01-23 19:01 ` Thomas Gleixner
2017-01-24 22:10 ` Shivappa Vikas
2017-01-10 19:33 ` [PATCH 2/8] x86/intel_rdt/mba: Generalize the naming to get ready for MBA Vikas Shivappa
2017-01-16 13:45 ` Thomas Gleixner
2017-01-10 19:33 ` [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA Vikas Shivappa
2017-01-16 13:54 ` Thomas Gleixner
2017-01-18 0:53 ` Shivappa Vikas
2017-01-18 9:05 ` Thomas Gleixner
2017-01-10 19:33 ` [PATCH 4/8] x86/intel_rdt/mba: Memory b/w allocation feature detect Vikas Shivappa
2017-01-16 13:59 ` Thomas Gleixner
2017-01-16 14:41 ` Peter Zijlstra
2017-01-16 16:16 ` Thomas Gleixner
2017-01-10 19:33 ` [PATCH 5/8] x86/intel_rct/mba: Add MBA structures and initialize MBA Vikas Shivappa
2017-01-16 14:06 ` Thomas Gleixner
2017-01-18 0:56 ` Shivappa Vikas
2017-01-10 19:33 ` [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA Vikas Shivappa
2017-01-16 14:14 ` Thomas Gleixner
2017-01-18 1:02 ` Shivappa Vikas
2017-01-18 9:08 ` Thomas Gleixner
2017-01-23 19:45 ` Shivappa Vikas
2017-01-10 19:33 ` [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support " Vikas Shivappa
2017-01-16 16:07 ` Thomas Gleixner
2017-01-18 1:03 ` Shivappa Vikas [this message]
2017-01-10 19:33 ` [PATCH 8/8] x86/intel_rdt: rmdir,umount and hotcpu updates " Vikas Shivappa
2017-01-16 16:13 ` Thomas Gleixner
2017-01-23 20:18 ` Shivappa Vikas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.10.1701171703140.15892@vshiva-Udesk \
--to=vikas.shivappa@intel.com \
--cc=fenghua.yu@intel.com \
--cc=h.peter.anvin@intel.com \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=ravi.v.shankar@intel.com \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=vikas.shivappa@linux.intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).