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 05/20] x86/resctrl: Create mba_sc configuration in the rdt_domain
Date: Fri, 17 Sep 2021 17:57:57 +0100	[thread overview]
Message-ID: <075ebc5d-4a04-b006-dba7-e33e1bf65b03@arm.com> (raw)
In-Reply-To: <638a0c73-ed1b-c4d6-f5f2-2af3c2e39a35@intel.com>

Hi Reinette,

On 01/09/2021 22:22, Reinette Chatre wrote:
> On 7/29/2021 3:35 PM, James Morse wrote:
>> To support resctrl's MBA software controller, the architecture must provide
>> a second configuration array to hold the mbps_val from user-space.
>>
>> This complicates the interface between the architecture code.
> 
> This sentence seems incomplete. I was expecting something like " complicates the interface
> between the architecture code and ..."

Yup, looks like I got distracted while writing that.


>> Make the filesystem parts of resctrl create an array for the mba_sc
>> values when the struct resctrl_schema is created. The software controller
>> can be changed to use this, allowing the architecture code to only
>> consider the values configured in hardware.

>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index cf0db0b7a5d0..185f9bb992d1 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2030,6 +2030,60 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>>                    struct rdtgroup *prgrp,
>>                    struct kernfs_node **mon_data_kn);
>>   +static int mba_sc_domain_allocate(struct rdt_resource *res,
>> +                  struct rdt_domain *d)
>> +{
>> +    u32 num_closid = closid_free_map_len;
>> +    int cpu = cpumask_any(&d->cpu_mask);
>> +    int i;
>> +
>> +    d->mba_sc = kcalloc_node(num_closid, sizeof(*d->mba_sc),
>> +                 GFP_KERNEL, cpu_to_node(cpu));
>> +    if (!d->mba_sc)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < num_closid; i++)
>> +        d->mba_sc[i].mbps_val = MBA_MAX_MBPS;
>> +
>> +    return 0;
>> +}
>> +
> 
> I had the same initial reaction as Jamie and noted your answer to him. Considering the
> intricate flow here could you please add some comments to these functions that explains
> the calling flows in support of their safety?

After a shuffle to make this allocated at a more obvious time, it has this comment:
|	/*
|	 * d->mbps_val is allocated by a call to this function in set_mba_sc(),
|	 * and domain_setup_mon_state(). Both calls are guarded by is_mba_sc(),
|	 * which can only return true while the filesystem is mounted. The
|	 * two calls are prevented from racing as rdt_get_tree() takes the
|	 * cpuhp read lock before calling rdt_enable_ctx(ctx), which prevents
|	 * it running concurrently with resctrl_online_domain().
|	 */


>>   /**
>>    * struct rdt_domain - group of CPUs sharing a resctrl resource
>>    * @list:        all instances of this resource
>> @@ -53,6 +64,7 @@ struct resctrl_staged_config {
>>    * @cqm_work_cpu:    worker CPU for CQM h/w counters
>>    * @plr:        pseudo-locked region (if any) associated with domain
>>    * @staged_config:    parsed configuration to be applied
>> + * @mba_sc:    the mba software controller properties, indexed by closid
>>    */
>>   struct rdt_domain {
>>       struct list_head        list;
>> @@ -67,6 +79,7 @@ struct rdt_domain {
>>       int                cqm_work_cpu;
>>       struct pseudo_lock_region    *plr;
>>       struct resctrl_staged_config    staged_config[CDP_NUM_TYPES];
>> +    struct resctrl_mba_sc        *mba_sc;
>>   };
> 
> Why is this additional abstraction needed? As I understand the usage struct resctrl_mba_sc
> would always only have the one member so why not have mbps_val within rdt_domain?

Probably because I thought some form of prev_bw/delta_bw would go in here too, once the
variables need to calculate bytes had moved to be part of the architecture specific code.
In the end it didn't make sense because there are two mbm_state arrays, so can't be a
single struct array.

I'll roll it out,


Thanks,

James

  reply	other threads:[~2021-09-17 16:58 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 [this message]
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
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=075ebc5d-4a04-b006-dba7-e33e1bf65b03@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 05/20] x86/resctrl: Create mba_sc configuration in the rdt_domain' \
    /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).