Re: [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare\

2017-03-30 Thread Shivappa Vikas 



On Wed, 1 Mar 2017, Thomas Gleixner wrote:


On Fri, 17 Feb 2017, Vikas Shivappa wrote:

Subject: x86/intel_rdt: schemata file support for MBA prepare

I have no idea what MBA prepare is. Is that yet another variant aside of
MBE?


Add support to introduce generic APIs for control validation and writing
QOS_MSRs for RDT resources. The control validation api is meant to
validate the control values like cache bit mask for CAT and memory b/w
percentage for MBA. A resource generic display format is also added and
used for the resources depending on whether its displayed in
hex/decimal.


The usual unpenetratable mess of random sentences lumped together.


diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 24de64c..8748b0d 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -77,6 +77,9 @@ struct rftype {
  * @default_ctrl:  Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:  Minimum number of consecutive bits to be set
  * in a cache bit mask
+ * @format_str:Per resource format string to show domain val


Can you please spell out words in comments instead of using random
abbreviations just as you see fit?


+ * @parse_ctrlval: Per resource API to parse the ctrl values


That's not an API. That's a function pointer.


+ * @msr_update:API to update QOS MSRs


Ditto.


  * @info_files:resctrl info files for the resource
  * @infofiles_len: Number of info files
  * @max_delay: Max throttle delay. Delay is the hardware
@@ -105,6 +108,9 @@ struct rdt_resource {
int cbm_len;
int min_cbm_bits;
u32 default_ctrl;
+   const char  *format_str;
+   int (*parse_ctrlval)(char *buf, struct rdt_resource *r);
+   void (*msr_update)  (void *a1, void *a2, struct rdt_resource *r);


void *a1, *a2? Dammit, both implementations (CAT and MBA) use the same
types. This just avoids type safety which does not magically come back by
your completely nonsensical typecasts inside the callback implementations.


+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)


And this is global because it's only used in this file, right?


+{
+   struct rdt_domain *d = (struct rdt_domain *)a2;
+   struct msr_param *m = (struct msr_param *)a1;


Oh well..


+   int i;
+
+   for (i = m->low; i < m->high; i++) {
+   int idx = cbm_idx(r, i);
+
+   wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+   }
+}



-static bool cbm_validate(unsigned long var, struct rdt_resource *r)
+static int cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 {
-   unsigned long first_bit, zero_bit;
+   unsigned long first_bit, zero_bit, var;
+   int ret;
+
+   ret = kstrtoul(buf, 16, );
+   if (ret)
+   return ret;

if (var == 0 || var > r->default_ctrl)
-   return false;
+   return -EINVAL;


So you change this function and the whole call chain to return either
-EINVAL or 0 instead of false/true.

And then you treat the integer return value as boolean on the call site
again:


Will fix wrt all the comments.

Thanks,
Vikas

Re: [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare\

2017-03-30 Thread Shivappa Vikas 



On Wed, 1 Mar 2017, Thomas Gleixner wrote:


On Fri, 17 Feb 2017, Vikas Shivappa wrote:

Subject: x86/intel_rdt: schemata file support for MBA prepare

I have no idea what MBA prepare is. Is that yet another variant aside of
MBE?


Add support to introduce generic APIs for control validation and writing
QOS_MSRs for RDT resources. The control validation api is meant to
validate the control values like cache bit mask for CAT and memory b/w
percentage for MBA. A resource generic display format is also added and
used for the resources depending on whether its displayed in
hex/decimal.


The usual unpenetratable mess of random sentences lumped together.


diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 24de64c..8748b0d 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -77,6 +77,9 @@ struct rftype {
  * @default_ctrl:  Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:  Minimum number of consecutive bits to be set
  * in a cache bit mask
+ * @format_str:Per resource format string to show domain val


Can you please spell out words in comments instead of using random
abbreviations just as you see fit?


+ * @parse_ctrlval: Per resource API to parse the ctrl values


That's not an API. That's a function pointer.


+ * @msr_update:API to update QOS MSRs


Ditto.


  * @info_files:resctrl info files for the resource
  * @infofiles_len: Number of info files
  * @max_delay: Max throttle delay. Delay is the hardware
@@ -105,6 +108,9 @@ struct rdt_resource {
int cbm_len;
int min_cbm_bits;
u32 default_ctrl;
+   const char  *format_str;
+   int (*parse_ctrlval)(char *buf, struct rdt_resource *r);
+   void (*msr_update)  (void *a1, void *a2, struct rdt_resource *r);


void *a1, *a2? Dammit, both implementations (CAT and MBA) use the same
types. This just avoids type safety which does not magically come back by
your completely nonsensical typecasts inside the callback implementations.


+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)


And this is global because it's only used in this file, right?


+{
+   struct rdt_domain *d = (struct rdt_domain *)a2;
+   struct msr_param *m = (struct msr_param *)a1;


Oh well..


+   int i;
+
+   for (i = m->low; i < m->high; i++) {
+   int idx = cbm_idx(r, i);
+
+   wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+   }
+}



-static bool cbm_validate(unsigned long var, struct rdt_resource *r)
+static int cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 {
-   unsigned long first_bit, zero_bit;
+   unsigned long first_bit, zero_bit, var;
+   int ret;
+
+   ret = kstrtoul(buf, 16, );
+   if (ret)
+   return ret;

if (var == 0 || var > r->default_ctrl)
-   return false;
+   return -EINVAL;


So you change this function and the whole call chain to return either
-EINVAL or 0 instead of false/true.

And then you treat the integer return value as boolean on the call site
again:


Will fix wrt all the comments.

Thanks,
Vikas

Re: [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare\

2017-03-01 Thread Thomas Gleixner 
On Fri, 17 Feb 2017, Vikas Shivappa wrote:

Subject: x86/intel_rdt: schemata file support for MBA prepare

I have no idea what MBA prepare is. Is that yet another variant aside of
MBE?

> Add support to introduce generic APIs for control validation and writing
> QOS_MSRs for RDT resources. The control validation api is meant to
> validate the control values like cache bit mask for CAT and memory b/w
> percentage for MBA. A resource generic display format is also added and
> used for the resources depending on whether its displayed in
> hex/decimal.

The usual unpenetratable mess of random sentences lumped together.

> diff --git a/arch/x86/include/asm/intel_rdt.h 
> b/arch/x86/include/asm/intel_rdt.h
> index 24de64c..8748b0d 100644
> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -77,6 +77,9 @@ struct rftype {
>   * @default_ctrl:Specifies default cache cbm or mem b/w percent.
>   * @min_cbm_bits:Minimum number of consecutive bits to be set
>   *   in a cache bit mask
> + * @format_str:  Per resource format string to show domain val

Can you please spell out words in comments instead of using random
abbreviations just as you see fit?

> + * @parse_ctrlval:   Per resource API to parse the ctrl values

That's not an API. That's a function pointer.

> + * @msr_update:  API to update QOS MSRs

Ditto.

>   * @info_files:  resctrl info files for the resource
>   * @infofiles_len:   Number of info files
>   * @max_delay:   Max throttle delay. Delay is the 
> hardware
> @@ -105,6 +108,9 @@ struct rdt_resource {
>   int cbm_len;
>   int min_cbm_bits;
>   u32 default_ctrl;
> + const char  *format_str;
> + int (*parse_ctrlval)(char *buf, struct rdt_resource *r);
> + void (*msr_update)  (void *a1, void *a2, struct rdt_resource *r);

void *a1, *a2? Dammit, both implementations (CAT and MBA) use the same
types. This just avoids type safety which does not magically come back by
your completely nonsensical typecasts inside the callback implementations.

> +void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)

And this is global because it's only used in this file, right?

> +{
> + struct rdt_domain *d = (struct rdt_domain *)a2;
> + struct msr_param *m = (struct msr_param *)a1;

Oh well..

> + int i;
> +
> + for (i = m->low; i < m->high; i++) {
> + int idx = cbm_idx(r, i);
> +
> + wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
> + }
> +}

> -static bool cbm_validate(unsigned long var, struct rdt_resource *r)
> +static int cbm_validate(char *buf, unsigned long *data, struct rdt_resource 
> *r)
>  {
> - unsigned long first_bit, zero_bit;
> + unsigned long first_bit, zero_bit, var;
> + int ret;
> +
> + ret = kstrtoul(buf, 16, );
> + if (ret)
> + return ret;
>  
>   if (var == 0 || var > r->default_ctrl)
> - return false;
> + return -EINVAL;

So you change this function and the whole call chain to return either
-EINVAL or 0 instead of false/true.

And then you treat the integer return value as boolean on the call site
again:

> @@ -90,7 +95,7 @@ static int parse_line(char *line, struct rdt_resource *r)
>   id = strsep(, "=");
>   if (kstrtoul(id, 10, _id) || dom_id != d->id)
>   return -EINVAL;
> - if (parse_cbm(dom, r))
> + if (r->parse_ctrlval(dom, r))
>   return -EINVAL;

What's the purpose of this exercise? Annoying reviewers or what?

Thanks,

tglx

Re: [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare\

2017-03-01 Thread Thomas Gleixner 
On Fri, 17 Feb 2017, Vikas Shivappa wrote:

Subject: x86/intel_rdt: schemata file support for MBA prepare

I have no idea what MBA prepare is. Is that yet another variant aside of
MBE?

> Add support to introduce generic APIs for control validation and writing
> QOS_MSRs for RDT resources. The control validation api is meant to
> validate the control values like cache bit mask for CAT and memory b/w
> percentage for MBA. A resource generic display format is also added and
> used for the resources depending on whether its displayed in
> hex/decimal.

The usual unpenetratable mess of random sentences lumped together.

> diff --git a/arch/x86/include/asm/intel_rdt.h 
> b/arch/x86/include/asm/intel_rdt.h
> index 24de64c..8748b0d 100644
> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -77,6 +77,9 @@ struct rftype {
>   * @default_ctrl:Specifies default cache cbm or mem b/w percent.
>   * @min_cbm_bits:Minimum number of consecutive bits to be set
>   *   in a cache bit mask
> + * @format_str:  Per resource format string to show domain val

Can you please spell out words in comments instead of using random
abbreviations just as you see fit?

> + * @parse_ctrlval:   Per resource API to parse the ctrl values

That's not an API. That's a function pointer.

> + * @msr_update:  API to update QOS MSRs

Ditto.

>   * @info_files:  resctrl info files for the resource
>   * @infofiles_len:   Number of info files
>   * @max_delay:   Max throttle delay. Delay is the 
> hardware
> @@ -105,6 +108,9 @@ struct rdt_resource {
>   int cbm_len;
>   int min_cbm_bits;
>   u32 default_ctrl;
> + const char  *format_str;
> + int (*parse_ctrlval)(char *buf, struct rdt_resource *r);
> + void (*msr_update)  (void *a1, void *a2, struct rdt_resource *r);

void *a1, *a2? Dammit, both implementations (CAT and MBA) use the same
types. This just avoids type safety which does not magically come back by
your completely nonsensical typecasts inside the callback implementations.

> +void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)

And this is global because it's only used in this file, right?

> +{
> + struct rdt_domain *d = (struct rdt_domain *)a2;
> + struct msr_param *m = (struct msr_param *)a1;

Oh well..

> + int i;
> +
> + for (i = m->low; i < m->high; i++) {
> + int idx = cbm_idx(r, i);
> +
> + wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
> + }
> +}

> -static bool cbm_validate(unsigned long var, struct rdt_resource *r)
> +static int cbm_validate(char *buf, unsigned long *data, struct rdt_resource 
> *r)
>  {
> - unsigned long first_bit, zero_bit;
> + unsigned long first_bit, zero_bit, var;
> + int ret;
> +
> + ret = kstrtoul(buf, 16, );
> + if (ret)
> + return ret;
>  
>   if (var == 0 || var > r->default_ctrl)
> - return false;
> + return -EINVAL;

So you change this function and the whole call chain to return either
-EINVAL or 0 instead of false/true.

And then you treat the integer return value as boolean on the call site
again:

> @@ -90,7 +95,7 @@ static int parse_line(char *line, struct rdt_resource *r)
>   id = strsep(, "=");
>   if (kstrtoul(id, 10, _id) || dom_id != d->id)
>   return -EINVAL;
> - if (parse_cbm(dom, r))
> + if (r->parse_ctrlval(dom, r))
>   return -EINVAL;

What's the purpose of this exercise? Annoying reviewers or what?

Thanks,

tglx

[PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare

2017-02-17 Thread Vikas Shivappa 
Add support to introduce generic APIs for control validation and writing
QOS_MSRs for RDT resources. The control validation api is meant to
validate the control values like cache bit mask for CAT and memory b/w
percentage for MBA. A resource generic display format is also added and
used for the resources depending on whether its displayed in
hex/decimal.

Signed-off-by: Vikas Shivappa 
---
 arch/x86/include/asm/intel_rdt.h |  8 +++
 arch/x86/kernel/cpu/intel_rdt.c  | 33 +-
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 40 ++--
 3 files changed, 58 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 24de64c..8748b0d 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -77,6 +77,9 @@ struct rftype {
  * @default_ctrl:  Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:  Minimum number of consecutive bits to be set
  * in a cache bit mask
+ * @format_str:Per resource format string to show domain val
+ * @parse_ctrlval: Per resource API to parse the ctrl values
+ * @msr_update:API to update QOS MSRs
  * @info_files:resctrl info files for the resource
  * @infofiles_len: Number of info files
  * @max_delay: Max throttle delay. Delay is the hardware
@@ -105,6 +108,9 @@ struct rdt_resource {
int cbm_len;
int min_cbm_bits;
u32 default_ctrl;
+   const char  *format_str;
+   int (*parse_ctrlval)(char *buf, struct rdt_resource *r);
+   void (*msr_update)  (void *a1, void *a2, struct rdt_resource *r);
struct rftype   *info_files;
int infofiles_len;
u32 max_delay;
@@ -150,6 +156,8 @@ struct msr_param {
 
 void rdt_get_cache_infofile(struct rdt_resource *r);
 void rdt_get_mba_infofile(struct rdt_resource *r);
+int parse_cbm(char *buf, struct rdt_resource *r);
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r);
 
 extern struct mutex rdtgroup_mutex;
 
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 353c476b4..7ce4453 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -44,6 +44,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L3",
.domains= domain_init(RDT_RESOURCE_L3),
.msr_base   = IA32_L3_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 3,
.cbm_idx_multi  = 1,
@@ -53,6 +56,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L3DATA",
.domains= domain_init(RDT_RESOURCE_L3DATA),
.msr_base   = IA32_L3_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 3,
.cbm_idx_multi  = 2,
@@ -62,6 +68,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L3CODE",
.domains= domain_init(RDT_RESOURCE_L3CODE),
.msr_base   = IA32_L3_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 3,
.cbm_idx_multi  = 2,
@@ -71,6 +80,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L2",
.domains= domain_init(RDT_RESOURCE_L2),
.msr_base   = IA32_L2_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 2,
.cbm_idx_multi  = 1,
@@ -258,11 +270,24 @@ static int get_cache_id(int cpu, int level)
return -1;
 }
 
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)
+{
+   struct rdt_domain *d = (struct rdt_domain *)a2;
+   struct msr_param *m = (struct msr_param *)a1;
+   int i;
+
+   for (i = m->low; i < m->high; i++) {
+   int idx = cbm_idx(r, i);
+
+   wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+   }
+}
+
 void rdt_ctrl_update(void *arg)
 {
struct msr_param *m = (struct msr_param *)arg;
struct rdt_resource *r = m->res;
-   int i, cpu = smp_processor_id();
+

[PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare

2017-02-17 Thread Vikas Shivappa 
Add support to introduce generic APIs for control validation and writing
QOS_MSRs for RDT resources. The control validation api is meant to
validate the control values like cache bit mask for CAT and memory b/w
percentage for MBA. A resource generic display format is also added and
used for the resources depending on whether its displayed in
hex/decimal.

Signed-off-by: Vikas Shivappa 
---
 arch/x86/include/asm/intel_rdt.h |  8 +++
 arch/x86/kernel/cpu/intel_rdt.c  | 33 +-
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 40 ++--
 3 files changed, 58 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 24de64c..8748b0d 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -77,6 +77,9 @@ struct rftype {
  * @default_ctrl:  Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:  Minimum number of consecutive bits to be set
  * in a cache bit mask
+ * @format_str:Per resource format string to show domain val
+ * @parse_ctrlval: Per resource API to parse the ctrl values
+ * @msr_update:API to update QOS MSRs
  * @info_files:resctrl info files for the resource
  * @infofiles_len: Number of info files
  * @max_delay: Max throttle delay. Delay is the hardware
@@ -105,6 +108,9 @@ struct rdt_resource {
int cbm_len;
int min_cbm_bits;
u32 default_ctrl;
+   const char  *format_str;
+   int (*parse_ctrlval)(char *buf, struct rdt_resource *r);
+   void (*msr_update)  (void *a1, void *a2, struct rdt_resource *r);
struct rftype   *info_files;
int infofiles_len;
u32 max_delay;
@@ -150,6 +156,8 @@ struct msr_param {
 
 void rdt_get_cache_infofile(struct rdt_resource *r);
 void rdt_get_mba_infofile(struct rdt_resource *r);
+int parse_cbm(char *buf, struct rdt_resource *r);
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r);
 
 extern struct mutex rdtgroup_mutex;
 
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 353c476b4..7ce4453 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -44,6 +44,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L3",
.domains= domain_init(RDT_RESOURCE_L3),
.msr_base   = IA32_L3_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 3,
.cbm_idx_multi  = 1,
@@ -53,6 +56,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L3DATA",
.domains= domain_init(RDT_RESOURCE_L3DATA),
.msr_base   = IA32_L3_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 3,
.cbm_idx_multi  = 2,
@@ -62,6 +68,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L3CODE",
.domains= domain_init(RDT_RESOURCE_L3CODE),
.msr_base   = IA32_L3_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 3,
.cbm_idx_multi  = 2,
@@ -71,6 +80,9 @@ struct rdt_resource rdt_resources_all[] = {
.name   = "L2",
.domains= domain_init(RDT_RESOURCE_L2),
.msr_base   = IA32_L2_CBM_BASE,
+   .parse_ctrlval  = parse_cbm,
+   .msr_update = cqm_wrmsr,
+   .format_str = "%d=%x",
.min_cbm_bits   = 1,
.cache_level= 2,
.cbm_idx_multi  = 1,
@@ -258,11 +270,24 @@ static int get_cache_id(int cpu, int level)
return -1;
 }
 
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)
+{
+   struct rdt_domain *d = (struct rdt_domain *)a2;
+   struct msr_param *m = (struct msr_param *)a1;
+   int i;
+
+   for (i = m->low; i < m->high; i++) {
+   int idx = cbm_idx(r, i);
+
+   wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+   }
+}
+
 void rdt_ctrl_update(void *arg)
 {
struct msr_param *m = (struct msr_param *)arg;
struct rdt_resource *r = m->res;
-   int i, cpu = smp_processor_id();
+   int cpu = smp_processor_id();