Re: [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare\
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\
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\
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\
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
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
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();