Re: [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA

[Shivappa Vikas]

On Mon, 16 Jan 2017, Thomas Gleixner wrote:

> On Tue, 10 Jan 2017, Vikas Shivappa wrote:
> > This patch does some changes to get ready to handle more resources like
> > Memory b/w allocation(MBA).
> >
> > -Update the control registers only when user changes the controls(cbm for
> > Cache resources and Mem b/w for memory). Hence not sending IPIs on all
> > domains when user updates the control vals.
> > -Introduce next_enabled_resource rather than looping through all
> > resources while parsing each schemata line. The order of resources
> > should be anyways the same as the root schemata.
> > -Return error as soon as we detect a resource not entering all domain
> > values in schemata rather than waiting till we parse all resources.
>
> That looks all like reasonable optimizations and I have a hard time to
> understand why this is a prerequisite for the bandwidth support.
>
> And each of these changes is independent so they should be in seperate
> patches.

Ok will split them. Its not a pre requisite for MBA. Should i send as a seperate 
series ?

> > +/*
> > + * Parameter r must be NULL or pointing to
> > + * a valid rdt_resource_all entry.
> > + * Points r to the next enabled RDT resource at the end.
> > + */
> > +#define next_enabled_rdt_resource(r)					\
> > +do {									\
> > +	if (!r)								\
> > +		r = rdt_resources_all;					\
> > +	else								\
> > +		r++;							\
> > +	for (; r < rdt_resources_all + RDT_NUM_RESOURCES; r++)		\
> > +		if (r->enabled)						\
> > +			break;						\
> > +} while (0)
>
>
> This is crap, really. What the heck is wrong with a proper function?
>
> static struct rdt_resource *get_next_enabled_resource(struct rdt_resource *r)
> {
> 	....
>
> 	return r;
> }

Will fix,

Thanks,
Vikas

> Thanks,
>
> 	tglx