linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: James Morse <james.morse@arm.com>
To: Reinette Chatre <reinette.chatre@intel.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>,
	H Peter Anvin <hpa@zytor.com>, Babu Moger <Babu.Moger@amd.com>,
	shameerali.kolothum.thodi@huawei.com,
	Jamie Iles <jamie@nuviainc.com>,
	D Scott Phillips OS <scott@os.amperecomputing.com>,
	lcherian@marvell.com, bobo.shaobowang@huawei.com
Subject: Re: [PATCH v1 06/20] x86/resctrl: Switch over to the resctrl mbps_val list
Date: Fri, 1 Oct 2021 17:02:42 +0100	[thread overview]
Message-ID: <88b641ff-4854-aad9-5e8c-7bc307330cbd@arm.com> (raw)
In-Reply-To: <b211c9c0-5f4c-75b2-2966-2bc1b848ab31@intel.com>

Hi Reinette,

On 17/09/2021 19:20, Reinette Chatre wrote:
> On 9/17/2021 9:57 AM, James Morse wrote:
>> On 01/09/2021 22:25, Reinette Chatre wrote:
>>> On 7/29/2021 3:35 PM, James Morse wrote:
>>>> Updates to resctrl's software controller follow the same path as
>>>> other configuration updates, but they don't modify the hardware state.
>>>> rdtgroup_schemata_write() uses parse_line() and the resource's
>>>> ctrlval_parse function to stage the configuration.
>>>> resctrl_arch_update_domains() then updates the mbps_val[] array
>>>> instead, and resctrl_arch_update_domains() skips the rdt_ctrl_update()
>>>> call that would update hardware.
>>>>
>>>> This complicates the interface between resctrl's filesystem parts
>>>> and architecture specific code. It should be possible for mba_sc
>>>> to be completely implemented by the filesystem parts of resctrl. This
>>>> would allow it to work on a second architecture with no additional code.
>>>>
>>>> Change parse_bw() to write the configuration value directly to the
>>>> mba_sc[] array in the domain structure. Change rdtgroup_schemata_write()
>>>> to skip the call to resctrl_arch_update_domains(), meaning all the
>>>> mba_sc specific code in resctrl_arch_update_domains() can be removed.
>>>> On the read-side, show_doms() and update_mba_bw() are changed to read
>>>> the mba_sc[] array from the domain structure. With this,
>>>> resctrl_arch_get_config() no longer needs to consider mba_sc resources.
>>>>
>>>> Change parse_bw() to write these values directly, meaning
>>>> rdtgroup_schemata_write() never needs to call update_domains()
>>>> for mba_sc resources.
>>
>>> The above paragraph seems to contain duplicate information from the paragraph that
>>> precedes it.
>>
>> Looks like two commit messages got combined. I've removed this, and the below paragraphs
>> as its already covered.
>>
>>
>>>> Get show_doms() to test is_mba_sc() and retrieve the value
>>>> directly, instead of using get_config() for the hardware value.
>>>>
>>>> This means the arch code's resctrl_arch_get_config() and
>>>> resctrl_arch_update_domains() no longer need to be aware of
>>>> mba_sc, and we can get rid of the update_mba_bw() code that
>>>> reaches into the hw_dom to get the msr value.
>>
>>>> @@ -406,6 +406,14 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>>>>          list_for_each_entry(s, &resctrl_schema_all, list) {
>>>>            r = s->res;
>>>> +
>>>> +        /*
>>>> +         * Writes to mba_sc resources update the software controller,
>>>> +         * not the control msr.
>>>> +         */
>>>> +        if (is_mba_sc(r))
>>>> +            continue;
>>>> +
>>>
>>> A few resources can be updated in a single write to the schemata file. It is thus possible
>>> to update the cache allocation resource as well as memory bandwidth allocation in a single
>>> write.
>>
>> i.e. echo "L3:0=7ff;1=7ff\nMB:0=100;1=50" > schemata
> 
> I do not think something like the above would show the issue. If you want to test this via
> the shell you need to use ANSI-C quoting. Adjusting what you show to something like:
> 
> echo -n $'L3:0=7ff;1=7ff\nMB:0=100;1=50\n'
> 
>>> As I understand this change in this scenario all configuration updates will be
>>> skipped, not just the memory bandwidth allocation ones.
>>
>> The loop is per-schema, so its not a problem for L2/L3. This would only be a problem if
>> the is_mba_sc() resource had multiple schema. Only CDP does this, which the MBA controls
>> don't support.


> The loop iterates through the entire buffer provided to the schemata file and the buffer
> could contain multiple schema. This is more typical when interacting with the schemata
> file with a SDK perhaps.

I think we are talking about different loops. The diff didn't include much context.
With more context:

| ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
| 				  char *buf, size_t nbytes, loff_t off)
| {
[...]
|
| 	while ((tok = strsep(&buf, "\n")) != NULL) {
| 		resname = strim(strsep(&tok, ":"));
| 		if (!tok) {
| 			rdt_last_cmd_puts("Missing ':'\n");
| 			ret = -EINVAL;
| 			goto out;
| 		}
|		if (tok[0] == '\0') {
| 			rdt_last_cmd_printf("Missing '%s' value\n", resname);
| 			ret = -EINVAL;
| 			goto out;
| 		}
| 		ret = rdtgroup_parse_resource(resname, tok, rdtgrp);
| 		if (ret)
| 			goto out;
| 	}

This is the loop that iterates over the buffer. A break in here would cause the problem
you describe.

|
| 	list_for_each_entry(s, &resctrl_schema_all, list) {
| 		r = s->res;
|
| 		/*
| 		 * Writes to mba_sc resources update the software controller,
| 		 * not the control msr.
| 		 */
| 		if (is_mba_sc(r))
| 			continue;
|
| 		ret = resctrl_arch_update_domains(r, rdtgrp->closid);
| 		if (ret)
| 			goto out;
| 	}

Whereas this one is per-schema. The continue skips the call to update the hardware for
mba_sc, because this will be done by update_mba_bw() when it is next called.

Updating multiple resources with one schema write would be dealt with by the first loop.
The whole buffer is parsed, (unless there is an error). This patch doesn't affect that.
The second loop is is about updating the hardware to match the freshly parsed config.


Thanks,

James

  reply	other threads:[~2021-10-01 16:02 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-29 22:35 [PATCH v1 00/20] x86/resctrl: Make resctrl_arch_rmid_read() return values in bytes James Morse
2021-07-29 22:35 ` [PATCH v1 01/20] x86/resctrl: Kill off alloc_enabled James Morse
2021-08-11 12:12   ` Jamie Iles
2021-09-01 21:18   ` Reinette Chatre
2021-07-29 22:35 ` [PATCH v1 02/20] x86/resctrl: Merge mon_capable and mon_enabled James Morse
2021-08-11 12:15   ` Jamie Iles
2021-08-11 15:16     ` James Morse
2021-07-29 22:35 ` [PATCH v1 03/20] x86/resctrl: Add domain online callback for resctrl work James Morse
2021-09-01 21:19   ` Reinette Chatre
2021-09-17 16:57     ` James Morse
2021-07-29 22:35 ` [PATCH v1 04/20] x86/resctrl: Add domain offline " James Morse
2021-08-11 16:10   ` Jamie Iles
2021-09-01 21:21   ` Reinette Chatre
2021-07-29 22:35 ` [PATCH v1 05/20] x86/resctrl: Create mba_sc configuration in the rdt_domain James Morse
2021-08-11 16:32   ` Jamie Iles
2021-08-31 16:24     ` James Morse
2021-09-01 21:22   ` Reinette Chatre
2021-09-17 16:57     ` James Morse
2021-07-29 22:35 ` [PATCH v1 06/20] x86/resctrl: Switch over to the resctrl mbps_val list James Morse
2021-09-01 21:25   ` Reinette Chatre
2021-09-17 16:57     ` James Morse
2021-09-17 18:20       ` Reinette Chatre
2021-10-01 16:02         ` James Morse [this message]
2021-07-29 22:35 ` [PATCH v1 07/20] x86/resctrl: Remove architecture copy of mbps_val James Morse
2021-09-01 21:26   ` Reinette Chatre
2021-09-17 16:57     ` James Morse
2021-07-29 22:35 ` [PATCH v1 08/20] x86/resctrl: Remove set_mba_sc()s control array re-initialisation James Morse
2021-07-29 22:35 ` [PATCH v1 09/20] x86/resctrl: Abstract and use supports_mba_mbps() James Morse
2021-09-01 21:27   ` Reinette Chatre
2021-09-17 16:57     ` James Morse
2021-09-24  6:23   ` tan.shaopeng
2021-10-01 16:01     ` James Morse
2021-07-29 22:36 ` [PATCH v1 10/20] x86/resctrl: Allow update_mba_bw() to update controls directly James Morse
2021-09-01 21:28   ` Reinette Chatre
2021-07-29 22:36 ` [PATCH v1 11/20] x86/resctrl: Calculate bandwidth from the total bytes counter James Morse
2021-09-01 21:31   ` Reinette Chatre
2021-09-17 16:58     ` James Morse
2021-07-29 22:36 ` [PATCH v1 12/20] x86/recstrl: Add per-rmid arch private storage for overflow and chunks James Morse
2021-08-11 17:14   ` Jamie Iles
2021-08-31 16:25     ` James Morse
2021-07-29 22:36 ` [PATCH v1 13/20] x86/recstrl: Allow per-rmid arch private storage to be reset James Morse
2021-09-24  6:34   ` tan.shaopeng
2021-10-01 16:01     ` James Morse
2021-07-29 22:36 ` [PATCH v1 14/20] x86/resctrl: Abstract __rmid_read() James Morse
2021-07-29 22:36 ` [PATCH v1 15/20] x86/resctrl: Pass the required parameters into resctrl_arch_rmid_read() James Morse
2021-07-29 22:36 ` [PATCH v1 16/20] x86/resctrl: Move mbm_overflow_count() " James Morse
2021-07-29 22:36 ` [PATCH v1 17/20] x86/resctrl: Move get_corrected_mbm_count() " James Morse
2021-07-29 22:36 ` [PATCH v1 18/20] x86/resctrl: Rename and change the units of resctrl_cqm_threshold James Morse
2021-07-29 22:36 ` [PATCH v1 19/20] x86/resctrl: Add resctrl_rmid_realloc_limit to abstract x86's boot_cpu_data James Morse
2021-07-29 22:36 ` [PATCH v1 20/20] x86/resctrl: Make resctrl_arch_rmid_read() return values in bytes 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=88b641ff-4854-aad9-5e8c-7bc307330cbd@arm.com \
    --to=james.morse@arm.com \
    --cc=Babu.Moger@amd.com \
    --cc=bobo.shaobowang@huawei.com \
    --cc=bp@alien8.de \
    --cc=fenghua.yu@intel.com \
    --cc=hpa@zytor.com \
    --cc=jamie@nuviainc.com \
    --cc=lcherian@marvell.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=reinette.chatre@intel.com \
    --cc=scott@os.amperecomputing.com \
    --cc=shameerali.kolothum.thodi@huawei.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v1 06/20] x86/resctrl: Switch over to the resctrl mbps_val list' \
    /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

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).