Re: [PATCH 10/24] x86/resctrl: Move the schema names into struct resctrl_schema
Hi Reinette, On 17/11/2020 23:11, Reinette Chatre wrote: > On 10/30/2020 9:11 AM, James Morse wrote: >> Move the names used for the schemata file out of the resource and >> into struct resctrl_schema. This allows one resource to have two >> different names, based on the other schema properties. >> >> This patch copies the names, eventually resctrl will generate them. > > Please remove "This patch". > >> >> Remove the arch code's max_name_width, this is now resctrl's >> problem. >> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> index a65ff53394ed..28d69c78c29e 100644 >> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > > ... > >> @@ -391,7 +389,7 @@ static void show_doms(struct seq_file *s, struct >> resctrl_schema >> *schema, int clo >> bool sep = false; >> u32 ctrl_val; >> - seq_printf(s, "%*s:", max_name_width, r->name); >> + seq_printf(s, "%*s:", RESCTRL_NAME_LEN, schema->name); > > From what I understand this changes what some users will see. In the original > code the > "max_name_width" is computed based on the maximum length of resources > supported. Systems > that only support MBA would thus show a schemata of: > > MB:0=100;1=100 > > I expect the above change would change the output to: > MB:0=100;1=100 Aha! Despite the comment - I've totally miss-understood what this code is doing. Thanks! James
Re: [PATCH 10/24] x86/resctrl: Move the schema names into struct resctrl_schema
Hi James,
On 10/30/2020 9:11 AM, James Morse wrote:
Move the names used for the schemata file out of the resource and
into struct resctrl_schema. This allows one resource to have two
different names, based on the other schema properties.
This patch copies the names, eventually resctrl will generate them.
Please remove "This patch".
Remove the arch code's max_name_width, this is now resctrl's
problem.
Signed-off-by: James Morse
---
arch/x86/kernel/cpu/resctrl/core.c| 9 ++---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 +++---
arch/x86/kernel/cpu/resctrl/internal.h| 2 +-
arch/x86/kernel/cpu/resctrl/rdtgroup.c| 17 -
include/linux/resctrl.h | 7 +++
5 files changed, 25 insertions(+), 20 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/core.c
b/arch/x86/kernel/cpu/resctrl/core.c
index 1ed5e04031e6..cda071009fed 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -37,10 +37,10 @@ DEFINE_MUTEX(rdtgroup_mutex);
DEFINE_PER_CPU(struct resctrl_pqr_state, pqr_state);
/*
- * Used to store the max resource name width and max resource data width
+ * Used to store the max resource data width
* to display the schemata in a tabular format
*/
-int max_name_width, max_data_width;
+int max_data_width;
/*
* Global boolean for rdt_alloc which is true if any
@@ -776,13 +776,8 @@ static int resctrl_offline_cpu(unsigned int cpu)
static __init void rdt_init_padding(void)
{
struct rdt_resource *r;
- int cl;
for_each_alloc_capable_rdt_resource(r) {
- cl = strlen(r->name);
- if (cl > max_name_width)
- max_name_width = cl;
-
if (r->data_width > max_data_width)
max_data_width = r->data_width;
}
The original code determines the maximum width based on resources
supported by the platform.
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index a65ff53394ed..28d69c78c29e 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
...
@@ -391,7 +389,7 @@ static void show_doms(struct seq_file *s, struct
resctrl_schema *schema, int clo
bool sep = false;
u32 ctrl_val;
- seq_printf(s, "%*s:", max_name_width, r->name);
+ seq_printf(s, "%*s:", RESCTRL_NAME_LEN, schema->name);
From what I understand this changes what some users will see. In the
original code the "max_name_width" is computed based on the maximum
length of resources supported. Systems that only support MBA would thus
show a schemata of:
MB:0=100;1=100
I expect the above change would change the output to:
MB:0=100;1=100
list_for_each_entry(dom, >domains, list) {
hw_dom = resctrl_to_arch_dom(dom);
if (sep)
...
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 8a12f4128209..9f71f0238239 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -15,6 +15,11 @@ int proc_resctrl_show(struct seq_file *m,
#endif
+/*
+ * The longest name we expect in the schemata file:
+ */
+#define RESCTRL_NAME_LEN 7
+
enum resctrl_conf_type {
CDP_BOTH,
CDP_CODE,
@@ -172,12 +177,14 @@ struct rdt_resource {
/**
* @list: Member of resctrl's schema list
+ * @names: Name to use in "schemata" file
s/names/name/?
* @conf_type:Type of configuration, e.g. code/data/both
* @res: The rdt_resource for this entry
* @num_closidNumber of CLOSIDs available for this resource
*/
struct resctrl_schema {
struct list_headlist;
+ charname[RESCTRL_NAME_LEN];
enum resctrl_conf_type conf_type;
struct rdt_resource *res;
u32 num_closid;
Reinette
Re: [PATCH 10/24] x86/resctrl: Move the schema names into struct resctrl_schema
Hi Jamie,
Thanks for taking a look,
On 10/11/2020 11:39, Jamie Iles wrote:
> On Fri, Oct 30, 2020 at 04:11:06PM +, James Morse wrote:
>> Move the names used for the schemata file out of the resource and
>> into struct resctrl_schema. This allows one resource to have two
>> different names, based on the other schema properties.
>>
>> This patch copies the names, eventually resctrl will generate them.
>>
>> Remove the arch code's max_name_width, this is now resctrl's
>> problem.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 311a3890bc53..48f4d6783647 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2150,6 +2151,12 @@ static int create_schemata_list(void)
>> s->num_closid = resctrl_arch_get_num_closid(r);
>> s->conf_type = resctrl_to_arch_res(r)->conf_type;
>>
>> +ret = snprintf(s->name, sizeof(s->name), r->name);
>> +if (ret >= sizeof(s->name)) {
>> +kfree(s);
>> +return -EINVAL;
>> +}
>> +
>
> How about:
>
> + ret = strscpy(s->name, r->name, sizeof(s->name));
> + if (ret < 0)) {
> + kfree(s);
> + return -EINVAL;
> + }
Never heard of it ... yup, that looks better. Thanks!
(I thought I knew not to write that bug!)
> So that there isn't a non-constant format specifier that'll trip
> Coverity+friends up later?
Heh, its gone by the last patch.
Fixed locally.
Thanks,
James
Re: [PATCH 10/24] x86/resctrl: Move the schema names into struct resctrl_schema
Hi James,
On Fri, Oct 30, 2020 at 04:11:06PM +, James Morse wrote:
> Move the names used for the schemata file out of the resource and
> into struct resctrl_schema. This allows one resource to have two
> different names, based on the other schema properties.
>
> This patch copies the names, eventually resctrl will generate them.
>
> Remove the arch code's max_name_width, this is now resctrl's
> problem.
>
> Signed-off-by: James Morse
> ---
> arch/x86/kernel/cpu/resctrl/core.c| 9 ++---
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 +++---
> arch/x86/kernel/cpu/resctrl/internal.h| 2 +-
> arch/x86/kernel/cpu/resctrl/rdtgroup.c| 17 -
> include/linux/resctrl.h | 7 +++
> 5 files changed, 25 insertions(+), 20 deletions(-)
...
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index 27671a654f8b..5294ae0c3ed9 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -248,7 +248,7 @@ struct rdtgroup {
> /* List of all resource groups */
> extern struct list_head rdt_all_groups;
>
> -extern int max_name_width, max_data_width;
> +extern int max_data_width;
>
> int __init rdtgroup_init(void);
> void __exit rdtgroup_exit(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 311a3890bc53..48f4d6783647 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1440,8 +1440,8 @@ static int rdtgroup_size_show(struct kernfs_open_file
> *of,
> rdt_last_cmd_puts("Cache domain offline\n");
> ret = -ENODEV;
> } else {
> - seq_printf(s, "%*s:", max_name_width,
> -rdtgrp->plr->s->res->name);
> + seq_printf(s, "%*s:", RESCTRL_NAME_LEN,
> +rdtgrp->plr->s->name);
> size = rdtgroup_cbm_to_size(rdtgrp->plr->s->res,
> rdtgrp->plr->d,
> rdtgrp->plr->cbm);
> @@ -1454,7 +1454,7 @@ static int rdtgroup_size_show(struct kernfs_open_file
> *of,
> r = schema->res;
>
> sep = false;
> - seq_printf(s, "%*s:", max_name_width, r->name);
> + seq_printf(s, "%*s:", RESCTRL_NAME_LEN, schema->name);
> list_for_each_entry(d, >domains, list) {
> hw_dom = resctrl_to_arch_dom(d);
> if (sep)
> @@ -1827,7 +1827,7 @@ static int rdtgroup_create_info_dir(struct kernfs_node
> *parent_kn)
> list_for_each_entry(s, _all_schema, list) {
> r = s->res;
> fflags = r->fflags | RF_CTRL_INFO;
> - ret = rdtgroup_mkdir_info_resdir(s, r->name, fflags);
> + ret = rdtgroup_mkdir_info_resdir(s, s->name, fflags);
> if (ret)
> goto out_destroy;
> }
> @@ -2140,6 +2140,7 @@ static int create_schemata_list(void)
> {
> struct resctrl_schema *s;
> struct rdt_resource *r;
> + int ret;
>
> for_each_alloc_enabled_rdt_resource(r) {
> s = kzalloc(sizeof(*s), GFP_KERNEL);
> @@ -2150,6 +2151,12 @@ static int create_schemata_list(void)
> s->num_closid = resctrl_arch_get_num_closid(r);
> s->conf_type = resctrl_to_arch_res(r)->conf_type;
>
> + ret = snprintf(s->name, sizeof(s->name), r->name);
> + if (ret >= sizeof(s->name)) {
> + kfree(s);
> + return -EINVAL;
> + }
> +
How about:
+ ret = strscpy(s->name, r->name, sizeof(s->name));
+ if (ret < 0)) {
+ kfree(s);
+ return -EINVAL;
+ }
So that there isn't a non-constant format specifier that'll trip
Coverity+friends up later?
Thanks,
Jamie
