Re: [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for MBA

[Shivappa Vikas]

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