Received-SPF: pass (google.com: domain of linux-kernel+bounces-180162-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <666470f0-e8c0-423e-bcb3-ea16359f5cfd@intel.com>
Date: Wed, 15 May 2024 09:47:28 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v17 7/9] x86/resctrl: Add new monitor files for Sub-NUMA
 cluster (SNC) monitoring
To: Tony Luck <tony.luck@intel.com>
CC: "Yu, Fenghua" <fenghua.yu@intel.com>, "Wieczor-Retman, Maciej"
	<maciej.wieczor-retman@intel.com>, Peter Newman <peternewman@google.com>,
	James Morse <james.morse@arm.com>, Babu Moger <babu.moger@amd.com>, "Drew
 Fustini" <dfustini@baylibre.com>, Dave Martin <Dave.Martin@arm.com>,
	"x86@kernel.org" <x86@kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "patches@lists.linux.dev"
	<patches@lists.linux.dev>
References: <20240503203325.21512-1-tony.luck@intel.com>
 <20240503203325.21512-8-tony.luck@intel.com>
 <0178e84e-d55f-47bf-b8b0-58e05fcaa108@intel.com>
 <ZkJIZdU2knEUJN7Q@agluck-desk3>
 <f49931b0-d9bf-45f0-ab35-93b1a78f6b97@intel.com>
 <ZkKupOKRu5S7Rkgx@agluck-desk3>
 <2efcae46-736a-4809-8530-7dde3977f3ce@intel.com>
 <SJ1PR11MB60838E2DB2984EF95D34C692FCE32@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <b718c70b-21ea-4c4d-9ecf-387b8276a721@intel.com>
 <ZkPdPTbgH-0EpBc4@agluck-desk3>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZkPdPTbgH-0EpBc4@agluck-desk3>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TEpEd0QwUWNTc21qaU0yR2RJYUdLZWgwUUNBb0l2eDlhVFBYZlluWlI1TDU5?=
 =?utf-8?B?eElRN20wR2FMQnZHaDZ4aUlOOG9EUmg4WjVPVFg5cFU3MkRmOWx0aU1Vc2gv?=
 =?utf-8?B?LzI0N2t6ZnJ0Z0p2YmRsZ081V1d3aml4UzdXR014NE1RN1lKeUZwaUJobnJ2?=
 =?utf-8?B?SHBSa0dZNGRKbVZDWEJhOGd3U0wrRnNkYzNjV05Pd3R4MDRMdjBZazZhdW1D?=
 =?utf-8?B?ZE1wakxnUDBhWXVZeW1mTSs1UFp6S2lraVg1VEtZdnY5MUZDMWc0bkdYYnRk?=
 =?utf-8?B?VjFwYzRJeTFEenNoZmVDOW9qQXFIT3BRVFkvZFc2d0ZmZm5vNjV2cE11MSs5?=
 =?utf-8?B?SFRHZGdjZHdiZHR3a1pxR0pqNnY2eXp3a2h6cVF6TThLdDRROFJGdWVkWDU5?=
 =?utf-8?B?RzlJUkFQcWowY1U3cGxTU0pwQ0pjOVhyTGlKUmRCcHJoRExYbUhmY05DZElP?=
 =?utf-8?B?blc3UWp3SmtuSTJZRmFYYXlOWW1KUnRXNjdreUZUMTdyUXFiWDlTZnR2dGdh?=
 =?utf-8?B?LzZXS1R2c0pSTXJXazAzb0FEN1cyNngxOVpwL3ZwejFCTU1jczZ5TEp4dUJP?=
 =?utf-8?B?cjM2cGo4MGhrUDdBdVZ0Qm5Pa0l4RjRtdTN4TWJTQy83VUgxUERkNTNzM1RQ?=
 =?utf-8?B?eTVoSjBvLzVRZ2VMclo1ekpjYkRJK2M5cXcvWGZ2WCtPM28yaUk4REh0bjU5?=
 =?utf-8?B?czR0czNEUUhiWDUxM0gvMFVxRis5a09acjhoUCtpdElkcXpSZ0lPSStyRDRN?=
 =?utf-8?B?ckVNMDRySFFlclV0UnVSZTdsTUpqQ1QxdkRvR1g3MmdzR3hrd0RxSzdYNy9L?=
 =?utf-8?B?Smtubk91bUp3WE1rcXB0QVJCWGJkOFV3dUJNTFNiYmtPaXd5UU5PRG5UMlcy?=
 =?utf-8?B?SXFHZ01aTlp4UlErZGtvWmEzYURYRE9WVnNIM1F5eGxxcVg3N0JPZ0VkWkw5?=
 =?utf-8?B?dGY0ZG5PMUdtUVlsYllQVTQ0YUtJdTNYRm04L00zZzRVWUY5bUFzMnN3SFRu?=
 =?utf-8?B?UG5RK3h0cUc3VmR6RTNVSW1VOE5JRktGcVdJQzZiL1pxZ2F4SHFXVkxEY3dr?=
 =?utf-8?B?a3hCUnRMeWQzZHZmNjlJdjYwRHZYM213Z2Vqa29SN0w0eGVkU2l5LzBTTzNW?=
 =?utf-8?B?VHhTcFNkcmdhZGlqdWZBL2hwOTVLT2U4cWIrbU94blkzMHFQQnFCQVM2Ukth?=
 =?utf-8?B?SjJVYlhmUGxoTzd5Tjk1aTFtVnFQNUg4R0pDZ2hJamFLVkUyaXMycWt4eEdH?=
 =?utf-8?B?Snltc1luZ0JyUG5rSk01dWNmb21xZWFKa1FqM2pvODdSTzJjQ2JPbkIzcDdP?=
 =?utf-8?B?a1U5VjZtNmZXYU43THErb1A2TW9mSnMvaklrOFJBbi9LRzRtNDJsVHNld1dQ?=
 =?utf-8?B?VldQeHdyOVJoTlNBL1RSblJLaldJblRjMWZUWW8yZTdCWk5DcjM1UVFmeWRj?=
 =?utf-8?B?MjNNZVlpWDBEZ0Jxd2ZvMGdORnIzbXd6ZjhkcXRPYW1kNzQxSXpxZC96Yi9Q?=
 =?utf-8?B?L1dLNnVGMm5mZlpkM0lwN3UvZzRIb3B5RVFEMmZYY2ZxL2FTQkI1MWtTREV2?=
 =?utf-8?B?d1hpNHZGaEFmbEVxTUdIYS8yQ3NRcmsyVGlQUWZYNGMzNGhWL3d0TEVQbDBu?=
 =?utf-8?B?SUFqeERtT0lTSDVyWFhNdUhlcGduV0RJeFlONUVOYzczVDAwM3YvcjNCc3gy?=
 =?utf-8?B?Nk9vS25KYmhRMTdJZm1BQkN6dDZSUEtZck90V0JTalZZWWhUelAwYVpuU1JF?=
 =?utf-8?B?RDBwa2YvS3lrUGh2dzBSMGhjSVBNd2wydmNMYnF5bWZVcG9TR1VMc2RTVUVU?=
 =?utf-8?B?QzZPUGZUMGF5TS9jMERwRG1qVzZGVEIrVDEwMmFLYmJ0UXMyaHdEY1pEUU12?=
 =?utf-8?B?R2pBbVQ4ekpxY2w1ZURhU0hDTjJTT2xzWGlyQzNQclhJbVh0aGNkdXNJbGVq?=
 =?utf-8?B?RkVZYWYzNjBPbXRxOE9IcFNhWXZLTHNTK0VhaXRBMGRkbFgvdnhWUjVTdW9y?=
 =?utf-8?B?MEsxUFhleUVLSlgremlDQzFCVWNoWGtaMzBGVDBrMTBFenpyNTFzZHpIV1pL?=
 =?utf-8?B?bkN3NGxKbzRiV3A3YTlnbS9hNzB0UWhQQmxoTzhCeVJMdGF2alV1MDZYbHBp?=
 =?utf-8?B?V0lkcTU4dTFzNU5oaml3MytyTlpQTEl5VnBtYkw1TUhiMGxGWkxueUZiQmlQ?=
 =?utf-8?B?MHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2f7e11d8-1e45-4535-627f-08dc74feb601
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2024 16:47:30.5380
 (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: WtZX07q48Mgk5YpgryM+e7AHKJR4q1W1SZl219L3F2KSZjBev8VnARR4Mu2OnzYYh5Dk5tDSHSoDAXrb5MatUwGn83WzygFvLmvopbudVYI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7399
X-OriginatorOrg: intel.com

Hi Tony,

On 5/14/2024 2:53 PM, Tony Luck wrote:
> On Tue, May 14, 2024 at 01:30:05PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 5/14/2024 11:26 AM, Luck, Tony wrote:
>>>> On 5/13/2024 5:21 PM, Tony Luck wrote:
>>>>> On Mon, May 13, 2024 at 11:53:17AM -0700, Reinette Chatre wrote:
>>>>>> On 5/13/2024 10:05 AM, Tony Luck wrote:
>>>>>>> On Fri, May 10, 2024 at 02:24:13PM -0700, Reinette Chatre wrote:
>>>>>>> Thanks for the review. Detailed comments below. But overall I'm
>>>>>>> going to split patch 7 into a bunch of smaller changes, each with
>>>>>>> a better commit message.
>>>>>>>
>>>>>>>> On 5/3/2024 1:33 PM, Tony Luck wrote:
>>>>>>>>
>>>>>>>> (Could you please start the changelog with some context?)
>>>>>>>>
>>>>>>>>> Add a field to the rdt_resource structure to track whether monitoring
>>>>>>>>> resources are tracked by hardware at a different scope (NODE) from
>>>>>>>>> the legacy L3 scope.
>>>>>>>>
>>>>>>>> This seems to describe @mon_scope that was introduced in patch #3?
>>>>>>>
>>>>>>> Not really. Patch #3 made the change so that control an monitor
>>>>>>> functions can have different scope. That's still needed as with SNC
>>>>>>> enabled the underlying data collection is at the node level for
>>>>>>> monitoring, while control stays at the L3 cache scope.
>>>>>>>
>>>>>>> This new field describes the legacy scope of monitoring, so that
>>>>>>> resctrl can provide correctly scoped monitor files for legacy
>>>>>>> applications that aren't aware of SNC. So I'm using this both
>>>>>>> to indicate when SNC is enabled (with mon_scope != mon_display_scope)
>>>>>>> or disabled (when they are the same).
>>>>>>
>>>>>> This seems to enforce the idea that these new additions aim to be
>>>>>> generic on the surface but the only goal is to support SNC.
>>>>>
>>>>> If you have some more ideas on how to make this more generic and
>>>>> less SNC specific I'm all ears.
>>>>
>>>> It may not end up being totally generic. It should not pretend to be
>>>> when it is not. It makes the flows difficult to follow when there are
>>>> these unexpected checks/quirks in what claims to be core code.
>>>
>>> Do you want some sort of warning comments in pieces of code
>>> that are SNC specific?
>>
>> I cannot think now where warnings will be appropriate but if you
>> find instances then please do. To start the quirks can at least be
>> documented. For example, "Only user of <feature> is SNC, which does
>> not require <custom> so simplify by <describe shortcut> ..."
> 
> The main spot that triggered this line of discussion was changing the
> sanity check that operations to read monitors is being done from a
> CPU within the right domain. I've added a short comment on the new
> check:
> 
> -       if (!cpumask_test_cpu(smp_processor_id(), &d->hdr.cpu_mask))
> +       /* Event counts can only be read from a CPU on the same L3 cache */
> +       if (d->display_id != get_cpu_cacheinfo_id(smp_processor_id(), r->mon_display_scope))
>                 return -EINVAL;
> 
> But my change embeds the assumption that monitor events are L3 scoped.
> 
> Should it be something like this (to keep the non-SNC case generic):
> 
> 	if (r->mon_scope == r->mon_display_scope) {
> 		if (!cpumask_test_cpu(smp_processor_id(), &d->hdr.cpu_mask))
> 			return -EINVAL;

Yes, keeping this check looks good to me ...

> 	} else {
> 		/*
> 		 * SNC: OK to read events on any CPU sharing same L3
> 		 * cache instance.
> 		 */
> 		 if (d->display_id != get_cpu_cacheinfo_id(smp_processor_id(), r->mon_display_scope))
> 		 	return -EINVAL;
> 	}

.. while I remain unsure about where "display_id" fits in.

> 
>>
>>>
>>>>
>>>>>>>>>         }
>>>>>>>>> +
>>>>>>>>> +       return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>>>>>>>>> +                               struct rdt_mon_domain *d,
>>>>>>>>> +                               struct rdt_resource *r, struct rdtgroup *prgrp)
>>>>>>>>> +{
>>>>>>>>> +       struct kernfs_node *kn, *ckn;
>>>>>>>>> +       char name[32];
>>>>>>>>> +       bool do_sum;
>>>>>>>>> +       int ret;
>>>>>>>>> +
>>>>>>>>> +       do_sum = r->mon_scope != r->mon_display_scope;
>>>>>>>>> +       sprintf(name, "mon_%s_%02d", r->name, d->display_id);
>>>>>>>>> +       kn = kernfs_find_and_get_ns(parent_kn, name, NULL);
>>>>>>>>> +       if (!kn) {
>>>>>>>>> +               /* create the directory */
>>>>>>>>> +               kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, prgrp);
>>>>>>>>> +               if (IS_ERR(kn))
>>>>>>>>> +                       return PTR_ERR(kn);
>>>>>>>>> +
>>>>>>>>> +               ret = rdtgroup_kn_set_ugid(kn);
>>>>>>>>> +               if (ret)
>>>>>>>>> +                       goto out_destroy;
>>>>>>>>> +               ret = mon_add_all_files(kn, d, r, prgrp, do_sum);
>>>>>>>>
>>>>>>>> This does not look right. If I understand correctly the private data
>>>>>>>> of these event files will have whichever mon domain came up first as
>>>>>>>> its domain id. That seems completely arbitrary and does not reflect
>>>>>>>> accurate state for this file. Since "do_sum" is essentially a "flag"
>>>>>>>> on how this file can be treated, can its "dom_id" not rather be
>>>>>>>> the "monitor scope domain id"? Could that not help to eliminate
>>>>>>>
>>>>>>> You are correct that this should be the "monitor scope domain id" rather
>>>>>>> than the first SNC domain that appears. I'll change to use that. I don't
>>>>>>> think it helps in removing the per-domain display_id.
>>>>>>
>>>>>> Wouldn't the file metadata then be the "display_id"?
>>>>>
>>>>> Yes. The metadata is the display_id for files that need to sum across
>>>>> SNC nodes, but the domain id for ones where no summation is needed.
>>>>
>>>> Right ... and there is a "sum" flag to tell which is which?
>>>
>>> Yes. sum==0 means the domid field is the one and only domain to
>>> report for this resctrl monitor file. sum==1 means the domid field is
>>> the display_id - all domains with this display_id must be summed to
>>> provide the result to present to the user.
>>>
>>> I've tried to capture that in the kerneldoc comment for struct mon_event.
>>> Here's what I'm planning to include in v18 (Outlook will probably mangle
>>> the formatting ... just imagine that the text lines up neatly):
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 49440f194253..3411557d761a 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -132,14 +132,19 @@ struct mon_evt {
>>>   *                     as kernfs private data
>>>   * @rid:               Resource id associated with the event file
>>>   * @evtid:             Event id associated with the event file
>>> - * @domid:             The domain to which the event file belongs
>>> + * @sum:               Set when event must be summed across multiple
>>> + *                     domains.
>>> + * @domid:             When @sum is zero this is the domain to which
>>> + *                     the event file belongs. When sum is one this
>>> + *                     is the display_id of all domains to be summed
>>
>> Here is where I would like to understand why it cannot just be
>> "When sum is one this is the domain id of the scope at which (for which?)
>> the events must be summed." Although, you already mentioned this will be
>> clear in next posting.
>>
>>>   * @u:                 Name of the bit fields struct
>>>   */
>>>  union mon_data_bits {
>>>         void *priv;
>>>         struct {
>>>                 unsigned int rid                : 10;
>>> -               enum resctrl_event_id evtid     : 8;
>>> +               enum resctrl_event_id evtid     : 7;
>>> +               unsigned int sum                : 1;
>>>                 unsigned int domid              : 14;
>>>         } u;
>>>  };
>>>
>>> -Tony
> 
> Maybe an example might help. Assume an SNC system with two sockets,
> three SNC nodes per socket, only supporting monitoring. The only domain
> list created by resctrl is the mon_domains list on the RDT_RESOURCE_L3
> resource. And it looks like this (with "disply_list" abbreviated to
> "dspl" to keep the picture small):
> 
> 
>        <------ SNC NODES ON SOCKET 0 ----->   <------ SNC NODES ON SOCKET 1 ------>
> ----> +----------+ +----------+ +----------+ +----------+ +----------+ +----------+
>       | id = 0   | | id = 1   | | id = 2   | | id = 3   | | id = 4   | | id = 5   |
>       |          | |          | |          | |          | |          | |          |
>       | dspl = 0 | | dspl = 0 | | dspl = 0 | | dspl = 1 | | dspl = 1 | | dspl = 1 |
>       |          | |          | |          | |          | |          | |          |
>       +----------+ +----------+ +----------+ +----------+ +----------+ +----------+
> 
> Reading the per-SNC node monitor values looks just the same as the
> non-SNC case. The struct rmid_read passed across the smp_call*() has
> the resource, domain, event, and reading the counters is essentially
> unchanged.
> 
> Reading a file to sum event counts for SNC nodes on socket 1 needs to
> find each of the "struct rdt_mon_domain" that are part of socket 1.
> I'm doing that with meta data in the file that says sum=1 (need to add
> up something) and domid=1 (the things to be added are those with
> display_id = 1). So the code reads:
> 
> 	list_for_each_entry(d, &rr->r->mon_domains, hdr.list) {
> 		if (d->display_id == rr->d->display_id) {
> 			... call stuff to read and sum for domain "d"
> 		}
> 	}
> 
> The display_id is "the domain id of the scope at which (for which?)
> the events must be summed." in your text above.

My point remains that it is not clear (to me) why it is required to
carry the display_id around.

 	list_for_each_entry(d, &rr->r->mon_domains, hdr.list) {
		/* determine @id of @d at rr->r->mon_display_scope */
 		if (id == domid) {
 			... call stuff to read and sum for domain "d"
 		}
 	}

Reinette