Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <28845d8b-cf7a-b5b7-d5ae-1284e33d063c@intel.com>
Date:   Thu, 27 Apr 2023 16:40:00 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.0
Subject: Re: [PATCH v3 11/19] x86/resctrl: Allow arch to allocate memory
 needed in resctrl_arch_rmid_read()
Content-Language: en-US
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>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        <carl@os.amperecomputing.com>, <lcherian@marvell.com>,
        <bobo.shaobowang@huawei.com>, <tan.shaopeng@fujitsu.com>,
        <xingxin.hx@openanolis.org>, <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>
References: <20230320172620.18254-1-james.morse@arm.com>
 <20230320172620.18254-12-james.morse@arm.com>
 <36af82d5-0d48-f899-9e95-1ec89be20581@intel.com>
 <24d3616a-7800-ba91-deed-8bcc639ce6ba@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <24d3616a-7800-ba91-deed-8bcc639ce6ba@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?L1MzR3lyK29tSkVsTDJLSTJ2WitUMTNiZjhSaWI5bGhYS0FCMlhjZWtPNkFz?=
 =?utf-8?B?TUJWTlQxdGlxUjV3R0NxMVByN2pIQzZKSURpalYvaHo0Z1U3bjFrN2lMZFVK?=
 =?utf-8?B?aFRVUHVPMFBOR1pZWG96QTJLbUVOcHJHckRjQ3U2bkJMdm9UUDhlSGJUa3Nu?=
 =?utf-8?B?U0MzZ0RzMGpCeGZIZTNtQ2tCcnpCZ3c2WVRmTHNKQWFtWVp2Y3B3NzNCZnpx?=
 =?utf-8?B?U3p5ZERIME15Z0RqREFkdGM3cGorOWY5UDRuMXRKQ3JTTWdpTnYrMFpFNFAx?=
 =?utf-8?B?UDY1ZHZ2RDhHSVZaRFpEa09EQWkra3E3NllVS3NFZHhHSzdmK25oRW9xTHZ2?=
 =?utf-8?B?b0VodGY3WmU3dFZ2bkNPdkl2dDRSNXFNNjhNRUh6WEhydk9TYlJKeS9tRzRl?=
 =?utf-8?B?Rk9vWmdEbkZsRzVxYnVzTlNlUXZzZHVCYjV6S3RFSU0wTXJwUDBQVm1ZbEFx?=
 =?utf-8?B?ek9TNC8zcmdTbjlDYjBsd1hCT0llTitaSThvVmZlYWx0TGNSYnhYVmhzUjBX?=
 =?utf-8?B?aDZwZ09aUmFVY0pOdVFCRzh4eE1CNG5VNFBTeStPTmQ5b1RGQnhiSENhRmVH?=
 =?utf-8?B?dk9jTXRtVSs2OE9qVUNmdXNQUFplejBwTjZsR0NKY2VhZkQxTDc1Rkh2Ukwv?=
 =?utf-8?B?RHRrN0NzSytJdkJ0cFBvdmRVZzlaU3FzUmMrbVZya2F1eVNmTTZDV3djek0w?=
 =?utf-8?B?REcvdjhrN0N0NjNHWWpvWUZPV2lZclVrSytUN3ErYWl5b25OVUE2bXNEUXFN?=
 =?utf-8?B?eExqUG9sajhDU1A3bkVxN0RXZHByV0ZFblFpZHg5b1k0NkVkcWxOWE1PNmdp?=
 =?utf-8?B?VmIyWkVSVHgzUDNpWWhJOXZMeFdFajVjeW1JVXRjM0hmTDB3UnV0ODVnVjhT?=
 =?utf-8?B?N25MbGtQYTc1Ty9TamtkVXkwZ0o1WHdvRTFxbjU3c3VhWmIrWmpsaDZCREtY?=
 =?utf-8?B?MDE0MzRRaml1dlZ2OXMycjdGWm9ERHZMN2hrSGhNZFpTMUl2RkpMMW9HL1NV?=
 =?utf-8?B?cUY1d0I3djJ4SnFuMFZLQWh4QmhkK1VydGwrQlBMVVZtUFIyd1RnT1VDVEpD?=
 =?utf-8?B?U215ci9vZHJ2WVRGYnRpallXRGI2MFczajRqemtVanQrUmRvVGcyclZFWFdV?=
 =?utf-8?B?T0Z3dElzS1hnVThKYit2bEFrT2NuWisrQ09WeUVCd0d5dGtxVmRSNnJtU0Mv?=
 =?utf-8?B?M3E0dXB5dWJsYWFRdzFHSVkzclEyWDRlZDBSaFZwek9ucXdnYUE3MjdKWW5Y?=
 =?utf-8?B?WHFuN0k5Y3JKbFFYdXhQU0UzeWdlTWNpWTJ6UXpzQmJvVVMwTEl2aFNJVWVj?=
 =?utf-8?B?aVJIeGhzbjEwWUcrVVdFUUFGeWp0V1NlcHd6OEpPeHc3RnlVL3dWdHdNTkdi?=
 =?utf-8?B?WHM0WlVtVWFQV29ueDZaWnhLUnNyTFFlTHoxc285Y05aR1AyT2k2Wm1TdWs1?=
 =?utf-8?B?NVRFZDdzMHVGMTNtcVA5RWkyQTJMcVoyQlZ2REdsOEtGY2xMSWpDd00rNzNX?=
 =?utf-8?B?MkpxaEhIQ3crMTBqNFlRZHNFalBGWWtnUU5YRTVpZkNaODE2NnFva1lTb3lm?=
 =?utf-8?B?c2RhRjkzdENML1hVV3JqeEU1TEJtUkhjdWJFdXlhVjNmcUdYSnNsaWMyVjNh?=
 =?utf-8?B?N2hWWjl0d0NUSWdUWFZDb1NDTzdTQzVoRkNDOURsVXY0Q216OEpSclM5a3Q3?=
 =?utf-8?B?dGpXMHZUR3llK0VLVzEzUFhXSlZkVWYyTHFwblBmR3loS0xySmNPK0w3TzEr?=
 =?utf-8?B?bVpzV1QyVi9sb0R0YVNhdTFkWkhuaWFjQU1mNmtQYS9BeHhWZjRpUng5TlFG?=
 =?utf-8?B?d0tNNDFYbFdlOXhZcnB5dlYzSStMQjkxVVpURnR6UzZ3Ty9JNmpDS3dleUpu?=
 =?utf-8?B?c0pMVkVsNFczV2NqN1c2SWhwR3V4ZFdacUUwdC9kTzRHZkpETDVJaHZRYjlu?=
 =?utf-8?B?bmN4NWdZNGZXZUkrQ0dYTkMzdmxaQzBjSGFINFJMaVJpYzlLSFpZSDNzc2o0?=
 =?utf-8?B?a2RMSExtVGoxdC9ZcTRIZHlzYThzUjdzVGpCenE1eEZLNzNoZ2tXMStRMTRF?=
 =?utf-8?B?MllWYnNZYXFpdVo1VXkrcmVodmVoWlNWQUo0ZUI0VnR5Y29YSkVmYzhYMTd4?=
 =?utf-8?B?YmNmcTU1MThWMzc1RWoyYzNmMktvOERBSDV5eVJySFM2eEhMTXIwWFdFZlh2?=
 =?utf-8?B?Tnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5e8b703f-d418-4355-3602-08db4778b8c3
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2023 23:40:02.6311
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EJDU/8PEBK9feh9V7viTThRDaYpSMKE+MQyC5RCWVr+3Mxs/Kw3SAqQsWWf3xgUJ7JM4J2PtrB3GOm+0bXNALTwjvMpqAMRZU2N0Zmd6yLA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5460
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi James,

On 4/27/2023 7:19 AM, James Morse wrote:
> On 01/04/2023 00:27, Reinette Chatre wrote:
>> On 3/20/2023 10:26 AM, James Morse wrote:

...

>>>  #include <linux/module.h>
>>>  #include <linux/sizes.h>
>>>  #include <linux/slab.h>
>>> @@ -271,7 +272,7 @@ static void smp_call_rmid_read(void *_arg)
>>>  
>>>  int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>>>  			   u32 closid, u32 rmid, enum resctrl_event_id eventid,
>>> -			   u64 *val)
>>> +			   u64 *val, int ignored)
>>>  {
>>>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>>  	struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
>>> @@ -317,9 +318,14 @@ void __check_limbo(struct rdt_domain *d, bool force_free)
>>>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>>>  	struct rmid_entry *entry;
>>>  	u32 idx, cur_idx = 1;
>>> +	int arch_mon_ctx;
>>>  	bool rmid_dirty;
>>>  	u64 val = 0;
>>>  
>>> +	arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, QOS_L3_OCCUP_EVENT_ID);
>>> +	if (arch_mon_ctx < 0)
>>> +		return;
> 
>> The vision for this is not clear to me. When I read that context needs to be allocated
>> I expect it to return a pointer to some new context, not an int. What would the
>> "context" consist of?
> 
> It might just need a different name.
> 
> For MPAM, this is allocating a monitor, which is the hardware that does the counting in
> the cache or the memory controller. The number of monitors is an implementation choice,
> and may not match the number of CLOSID/RMID that are in use. There aren't guaranteed to be
> enough to allocate one for every control or monitor group up front.
> 
> The int being returned is the allocated monitor's index. It identifies which monitor needs
> programming to read the provided CLOSID/RMID, and the counter register to read with the value.

I see.

> 
> I can allocate memory for an int if you think that is clearer.
> (I was hoping to leave that for whoever needs something bigger than a pointer)

I'd rather not complicate it in this way.

>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index ff7452f644e4..03e4f41cd336 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -233,6 +233,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d);
>>>   * @rmid:		rmid of the counter to read.
>>>   * @eventid:		eventid to read, e.g. L3 occupancy.
>>>   * @val:		result of the counter read in bytes.
>>> + * @arch_mon_ctx:	An allocated context from resctrl_arch_mon_ctx_alloc().
>>>   *
> 
>> Could this description be expanded to indicate what this context is used for?
> 
> Sure,
> "An architecture specific value from resctrl_arch_mon_ctx_alloc(), for MPAM this
> identifies the hardware monitor allocated for this read request".

This helps. Thank you.

Reinette