From: Reinette Chatre <reinette.chatre@intel.com>
To: James Morse <james.morse@arm.com>,
x86@kernel.org, linux-kernel@vger.kernel.org
Cc: Fenghua Yu <fenghua.yu@intel.com>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
shameerali.kolothum.thodi@huawei.com,
Jamie Iles <jamie@nuviainc.com>,
D Scott Phillips OS <scott@os.amperecomputing.com>
Subject: Re: [PATCH 08/24] x86/resctrl: Walk the resctrl schema list instead of an arch list
Date: Tue, 17 Nov 2020 14:52:33 -0800 [thread overview]
Message-ID: <cb95b485-4d34-fa88-7c66-2d4d6fc62b5d@intel.com> (raw)
In-Reply-To: <20201030161120.227225-9-james.morse@arm.com>
Hi James,
On 10/30/2020 9:11 AM, James Morse wrote:
> Now that resctrl has its own list of resources it is using, walk that
> list instead of the architectures list. This means resctrl has somewhere
> to keep schema properties with the resource that is using them.
>
> Most users of for_each_alloc_enabled_rdt_resource() are per-schema,
> and also want a schema property, like the conf_type. Switch these to
> walk the schema list. Schema were only created for alloc_enabled
> resources so these two lists are currently equivalent.
>
From what I understand based on this description the patch will
essentially change instances of for_each_alloc_enabled_rdt_resource() to
walking the schema list ....
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 38 ++++++++++++++---------
> arch/x86/kernel/cpu/resctrl/internal.h | 6 ++--
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 34 +++++++++++++-------
> include/linux/resctrl.h | 5 +--
> 4 files changed, 53 insertions(+), 30 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 8ac104c634fe..d3f9d142f58a 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -57,9 +57,10 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
> return true;
> }
>
> -int parse_bw(struct rdt_parse_data *data, struct rdt_resource *r,
> +int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
> struct rdt_domain *d)
> {
> + struct rdt_resource *r = s->res;
> unsigned long bw_val;
>
> if (d->have_new_ctrl) {
... this change and also the ones to parse_cbm() and
rdtgroup_cbm_overlaps() are not clear to me because it seems they
replace the rdt_resource parameter with resctrl_schema, but all in turn
use that to access rdt_resource again. That seems unnecessary?
> @@ -125,10 +126,11 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> * Read one cache bit mask (hex). Check that it is valid for the current
> * resource type.
> */
> -int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
> +int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
> struct rdt_domain *d)
> {
> struct rdtgroup *rdtgrp = data->rdtgrp;
> + struct rdt_resource *r = s->res;
> u32 cbm_val;
>
> if (d->have_new_ctrl) {
Really needed?
> @@ -160,12 +162,12 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
> * The CBM may not overlap with the CBM of another closid if
> * either is exclusive.
> */
> - if (rdtgroup_cbm_overlaps(r, d, cbm_val, rdtgrp->closid, true)) {
> + if (rdtgroup_cbm_overlaps(s, d, cbm_val, rdtgrp->closid, true)) {
> rdt_last_cmd_puts("Overlaps with exclusive group\n");
> return -EINVAL;
> }
>
> - if (rdtgroup_cbm_overlaps(r, d, cbm_val, rdtgrp->closid, false)) {
> + if (rdtgroup_cbm_overlaps(s, d, cbm_val, rdtgrp->closid, false)) {
> if (rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
> rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> rdt_last_cmd_puts("Overlaps with other group\n");
Needed?
> @@ -185,9 +187,10 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
> * separated by ";". The "id" is in decimal, and must match one of
> * the "id"s for this resource.
> */
> -static int parse_line(char *line, struct rdt_resource *r,
> +static int parse_line(char *line, struct resctrl_schema *s,
> struct rdtgroup *rdtgrp)
> {
> + struct rdt_resource *r = s->res;
> struct rdt_parse_data data;
> char *dom = NULL, *id;
> struct rdt_domain *d;
> @@ -213,7 +216,8 @@ static int parse_line(char *line, struct rdt_resource *r,
> if (d->id == dom_id) {
> data.buf = dom;
> data.rdtgrp = rdtgrp;
> - if (r->parse_ctrlval(&data, r, d))
> +
> + if (r->parse_ctrlval(&data, s, d))
> return -EINVAL;
> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>
needed?
/*
> @@ -289,10 +293,12 @@ static int rdtgroup_parse_resource(char *resname, char *tok,
> struct resctrl_schema *s;
> struct rdt_resource *r;
>
> + lockdep_assert_held(&rdtgroup_mutex);
> +
It is not clear how this addition fits into patch.
> list_for_each_entry(s, &resctrl_all_schema, list) {
> r = s->res;
> if (!strcmp(resname, r->name) && rdtgrp->closid < s->num_closid)
> - return parse_line(tok, r, rdtgrp);
> + return parse_line(tok, s, rdtgrp);
> }
needed? (similar comments to other changes in this patch but I will stop
here)
> rdt_last_cmd_printf("Unknown or unsupported resource name '%s'\n", resname);
> return -EINVAL;
...
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 628e5eb4d7a9..592a517afd6a 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
...
> @@ -2832,14 +2840,18 @@ static void rdtgroup_init_mba(struct rdt_resource *r)
> /* Initialize the RDT group's allocations. */
> static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
> {
> + struct resctrl_schema *s;
> struct rdt_resource *r;
> int ret;
>
> - for_each_alloc_enabled_rdt_resource(r) {
> + lockdep_assert_held(&rdtgroup_mutex);
> +
Another addition that does not match patch description.
> + list_for_each_entry(s, &resctrl_all_schema, list) {
> + r = s->res;
> if (r->rid == RDT_RESOURCE_MBA) {
> rdtgroup_init_mba(r);
> } else {
> - ret = rdtgroup_init_cat(r, rdtgrp->closid);
> + ret = rdtgroup_init_cat(s, rdtgrp->closid);
> if (ret < 0)
> return ret;
> }
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 20d8b6dd4af4..8a12f4128209 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
...
> @@ -171,7 +172,7 @@ struct rdt_resource {
>
> /**
> * @list: Member of resctrl's schema list
> - * @cdp_type: Whether this entry is for code/data/both
> + * @conf_type: Type of configuration, e.g. code/data/both
> * @res: The rdt_resource for this entry
> * @num_closid Number of CLOSIDs available for this resource
> */
>
This hunk can be merged with previous patch.
Reinette
next prev parent reply other threads:[~2020-11-17 22:52 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-30 16:10 [PATCH 00/24] x86/resctrl: Merge the CDP resources James Morse
2020-10-30 16:10 ` [PATCH 01/24] x86/resctrl: Split struct rdt_resource James Morse
2020-11-17 19:20 ` Reinette Chatre
2021-03-12 17:10 ` James Morse
2020-10-30 16:10 ` [PATCH 02/24] x86/resctrl: Split struct rdt_domain James Morse
2020-11-17 19:22 ` Reinette Chatre
2020-10-30 16:10 ` [PATCH 03/24] x86/resctrl: Add resctrl_arch_get_num_closid() James Morse
2020-11-17 19:57 ` Reinette Chatre
2021-03-12 17:10 ` James Morse
2020-10-30 16:11 ` [PATCH 04/24] x86/resctrl: Add a separate schema list for resctrl James Morse
2020-11-17 21:29 ` Reinette Chatre
2021-03-12 17:10 ` James Morse
2020-10-30 16:11 ` [PATCH 05/24] x86/resctrl: Pass the schema in resdir's private pointer James Morse
2020-11-17 21:49 ` Reinette Chatre
2021-03-12 17:11 ` James Morse
2020-10-30 16:11 ` [PATCH 06/24] x86/resctrl: Store the effective num_closid in the schema James Morse
2020-11-17 22:04 ` Reinette Chatre
2021-03-12 17:13 ` James Morse
2020-10-30 16:11 ` [PATCH 07/24] x86/resctrl: Label the resources with their configuration type James Morse
2020-11-17 22:30 ` Reinette Chatre
2021-03-12 17:36 ` James Morse
2020-10-30 16:11 ` [PATCH 08/24] x86/resctrl: Walk the resctrl schema list instead of an arch list James Morse
2020-11-17 22:52 ` Reinette Chatre [this message]
2021-03-12 17:37 ` James Morse
2020-10-30 16:11 ` [PATCH 09/24] x86/resctrl: Change rdt_resource to resctrl_schema in pseudo_lock_region James Morse
2020-10-30 16:11 ` [PATCH 10/24] x86/resctrl: Move the schema names into struct resctrl_schema James Morse
2020-11-10 11:39 ` Jamie Iles
2020-11-11 18:11 ` James Morse
2020-11-17 23:11 ` Reinette Chatre
2021-03-12 17:38 ` James Morse
2020-10-30 16:11 ` [PATCH 11/24] x86/resctrl: Group staged configuration into a separate struct James Morse
2020-11-17 23:28 ` Reinette Chatre
2021-03-12 17:41 ` James Morse
2020-10-30 16:11 ` [PATCH 12/24] x86/resctrl: Add closid to the staged config James Morse
2020-11-17 23:46 ` Reinette Chatre
2021-03-12 17:43 ` James Morse
2020-10-30 16:11 ` [PATCH 13/24] x86/resctrl: Allow different CODE/DATA configurations to be staged James Morse
2020-11-18 0:30 ` Reinette Chatre
2020-10-30 16:11 ` [PATCH 14/24] x86/resctrl: Make update_domains() learn the affected closids James Morse
2020-10-30 16:11 ` [PATCH 15/24] x86/resctrl: Add a helper to read a closid's configuration James Morse
2020-10-30 16:11 ` [PATCH 16/24] x86/resctrl: Add a helper to read/set the CDP configuration James Morse
2020-10-30 16:11 ` [PATCH 17/24] x86/resctrl: Use cdp_enabled in rdt_domain_reconfigure_cdp() James Morse
2020-10-30 16:11 ` [PATCH 18/24] x86/resctrl: Pass configuration type to resctrl_arch_get_config() James Morse
2020-10-30 16:11 ` [PATCH 19/24] x86/resctrl: Make ctrlval arrays the same size James Morse
2020-10-30 16:11 ` [PATCH 20/24] x86/resctrl: Apply offset correction when config is staged James Morse
2020-10-30 16:11 ` [PATCH 21/24] x86/resctrl: Calculate the index from the configuration type James Morse
2020-10-30 16:11 ` [PATCH 22/24] x86/resctrl: Merge the ctrlval arrays James Morse
2020-10-30 16:11 ` [PATCH 23/24] x86/resctrl: Remove rdt_cdp_peer_get() James Morse
2020-10-30 16:11 ` [PATCH 24/24] x86/resctrl: Merge the CDP resources James Morse
2020-11-13 15:38 ` [PATCH 00/24] " Jamie Iles
2020-11-16 17:54 ` Reinette Chatre
2020-11-17 13:05 ` James Morse
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cb95b485-4d34-fa88-7c66-2d4d6fc62b5d@intel.com \
--to=reinette.chatre@intel.com \
--cc=bp@alien8.de \
--cc=fenghua.yu@intel.com \
--cc=james.morse@arm.com \
--cc=jamie@nuviainc.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=scott@os.amperecomputing.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).