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
