Received-SPF: pass (google.com: domain of linux-kernel+bounces-180279-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <8c1fa448-2b75-4fca-8379-8f18e06861bd@intel.com>
Date: Wed, 15 May 2024 11:48:52 -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-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>
 <666470f0-e8c0-423e-bcb3-ea16359f5cfd@intel.com>
 <ZkTvdtzhlCg7SOQo@agluck-desk3>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZkTvdtzhlCg7SOQo@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?TUV0bS9iU3pBSHI3d0oybTNPT1NuOXFQT2Vwd0FPTFBzbERHSnhNckdWZDJa?=
 =?utf-8?B?a09jUmd5Z1hHaTlUOFVFeEJYTHVrWXVzOFBDcnFtV2g4YkNaM1Q5cUJlRmhU?=
 =?utf-8?B?cTBzNVRxMmRZcGRoT2pOV3gzZXgycjVCZ0ZzQkZOSkttZDN4dm1PUFYybjJD?=
 =?utf-8?B?MDNuWkxCd0tXMHJMQ1k5KzR0akVkMGlhaU9SVmZKOFo0djNHaDh5aTNGWGpF?=
 =?utf-8?B?RWVnUXlPNUFoT1lXcS9xM3JqOGp4V21GeTkyTzhub0FtYnVsclp6UUw4Zmgx?=
 =?utf-8?B?Q1NoUUhTNHFCa1VYakFubSt4S3ZaQ2w3aU5jZDRvVGhDVk5GRDRGZXZ0SUVL?=
 =?utf-8?B?S2pFRDVGTXJsbWJkVXBXeHpHRkY2UDBCYjhZb1dTVHZrQ2FjOEFrcVJJNlc5?=
 =?utf-8?B?RXp0ZUc0NEcvTWxNclVOeFB4cklyUGN5bThqTE03azlKMDBha2VQNmpuOEJ2?=
 =?utf-8?B?LzhVMUJkZDYxbVB3ZXd3R0x2WkQzNE1RWXVlYUZwTmExSGJIR1F2TTRteFB1?=
 =?utf-8?B?YlhNOWRmMERreUxyc1BtZk50V2cxYkJwZThmcTZBblBYSDJmVXVianRTU2FH?=
 =?utf-8?B?MDdiRFRjandwT1NiWHV6WDl6czJYQ3RxNnY2VStqUnFnY29URDlKK3NuZEVE?=
 =?utf-8?B?TTVEUXI0ODVQY3psYnJxNDY0aVU1ZlJYTWZqR2JhQVVDZDJBZWYrQ1VkbUlF?=
 =?utf-8?B?RlRpdDZjTnRVYXAwSllndFY4VTBLUEhGMnF6a3Rzdk5vR0UrWVltWWJUeXM2?=
 =?utf-8?B?OEZUb09QclJSV0xzSTdFWlpzZmFoaEtqdC9TWnZ1MTR5Y0NVV0RxVXlOcXg2?=
 =?utf-8?B?UEVBQTBMTmFidy9pd1JVeXFRanZsenkrYzAwRWs3TEpSU2JPV3ptQXdlemFm?=
 =?utf-8?B?SGQxRWdVdzRpUzJONVRyVGFDS0NPd2RMWXVvMXZyTDdmZzJVeE9JeXNlblZm?=
 =?utf-8?B?RmNuRDNOWTFrdlFacWdKQ2ZIV1NMN3VZYkVKS1hlZzVYQzZHZllIZUJpcTNV?=
 =?utf-8?B?VjlrOTg0WkpKS1NtamZSSjJ2Z2tQQWw3b1FocU4yQk1sZjNZUTNRcExBdC81?=
 =?utf-8?B?TG1RRGVTZUhJdFVYVTMrRTNPTnk5SjBVdXZ5b1FidHcvaG14eEFpM2FyRys4?=
 =?utf-8?B?VGpxNGNZTzdqa1BvZ1JVUncvUnloVjRYQ25vb1U2UUpZb0c1Y04vUTFnQ3N4?=
 =?utf-8?B?Y0V1Y0lacWdPMzg1TW9wSnovbGNrS3Zpa0tnSVZzb1RDaUtOOXVOcFhsT0l5?=
 =?utf-8?B?c3MvUjhJcTcyNjM1cnZyTFV6YTZWNEVRSEw1TEFyUWU1U1ltU3E5WEI2SmlF?=
 =?utf-8?B?WSs1enlrbFhQeUVVSitBUS9sRjNXTDEwYjlvalpBSGltMGdTMEZXbDIxRTc1?=
 =?utf-8?B?ejl5MkM5WlMyaSs2dHVWMUMxZVJNNTRzcjZaY0lFRE1RaUMvYmJrSCtTOWR4?=
 =?utf-8?B?RTZrMXFyZFBLcXJzRUU1S2g0NWFQSng1QTdZRVJqWmlXbG8xKzgxUU00MklQ?=
 =?utf-8?B?TjU2SU53dmJKbVdUei9nZmFnV2JjK2xDUmNtVm44MEZHRWE2MHFIYlQ5Szlx?=
 =?utf-8?B?VjRzb21FOE5oVWFZdkhXQzh2ZlNoWlJTSWN6Z0FUNlluZzhMYktnMWZUNGdx?=
 =?utf-8?B?d3YwcUkyQW1acTR5UlRPV3pONzJxYXpCV1RObXNKNGpKeDgzWDBnSEdaMVAx?=
 =?utf-8?B?YmtocTJpaEErQy8xTEFIOFhmamNhVHJPSnBxQUxBeHRtbmhoREJnQzRKRGdl?=
 =?utf-8?B?c2lFSjRYSHVuOG85MDlNQWVIV0l1Y3FyK2JHVUtIUjY5c0lYYjZNL0hKQWZl?=
 =?utf-8?B?VnRmQlA2SDZDMDZ6SE1NL0JiemdUVWJPTElDWUdmdmhwS2FXaGVxRVd2dU42?=
 =?utf-8?B?Yy9tT3pGenBUcng2NGxwTDcxVkpMVythVis5L0Q5UU1PcTdmbDc1WmRPa0tu?=
 =?utf-8?B?Mi96MG9YdGFwVURyNExFN3p4QTRvQXkyRTBzZGt4ZktZOTcvaGd1TXFBaExM?=
 =?utf-8?B?TEduRmkrTkQxYWJXZy9qMm4yY042azBtYVFoY0czcVlsdGhxTE1PQWVaU2JF?=
 =?utf-8?B?cnV0ZXoyK3pRWURoVEtYRGRGeFRNSzVweVFuZ2ZwVllsSzJTQndFYmxEeGdN?=
 =?utf-8?B?SFd2OU9COE1MdjVzc2ZzYy9sYU9QQ29oLyt2azdXZkVRMGxWeWtGeHdPbWIx?=
 =?utf-8?B?SVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3303249b-cb59-4401-8d50-08dc750fabb9
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2024 18:48:54.7197
 (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: ItuzfV2KkHRifvLFI4jx4xh9bGhREgLlvIuU0iVvizAuUqNfmR1OT3d7JtCTgoPwE/oDfqVzOQGtppmITGCsxIjkOupgNfPgsmmFEegbNlI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7964
X-OriginatorOrg: intel.com

Hi Tony,

On 5/15/2024 10:23 AM, Tony Luck wrote:
> On Wed, May 15, 2024 at 09:47:28AM -0700, Reinette Chatre wrote:
>> 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.
> 
> See below.
> 
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>>>>>         }
>>>>>>>>>>> +
>>>>>>>>>>> +       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"
>>  		}
>>  	}
> 
> That "determine @id of @d at rr->r->mon_display_scope" is:
> 
> 	display_id = get_domain_id_from_scope(cpumask_first(rr->d->hdr.cpu_mask), rr->r->mon_display_scope);
> 	if (display_id < 0) {
> 		take some error action
> 	}
> 
> So it certainly isn't *required* to carry display_id around. But doing
> so makes the code simpler. I could bury the long line into a helper

Is "if (d->display_id == rr->d->display_id)" really "simpler"? It is
shorter I agree, but I would argue that it is much harder to understand
what the code is trying to do. The reader needs to understand what
"display_id" means, how the state is maintained, how
the values propagated to this call site, etc. With a query like above
it should be obvious what the code does.

> macro/function. But I can't bury the error check.

If this is an error then it is a kernel bug and should be handled
appropriately.

> 
> I'd also need to change get_domain_id_from_scope() from "static" to
> global so it can be used in other files besides core.c

Is this a problem?

> Note that there are several places where I need to use display_id,
> computing it at run time in each place, but it seems so much easier to
> do it once at domain creation time.

Easier to code perhaps but I do not see how it is "easy" to understand
and maintain.

I think we have now repeated the same conversation twice. Previously you
promised that your design would be clear to me in the next version and
I have already stated twice that I am ok with that.

Reinette