Re: [PATCH v17 7/9] x86/resctrl: Add new monitor files for Sub-NUMA cluster (SNC) monitoring

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

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.

>>>>>  	}
>>>>> +
>>>>> +	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 
>>>> that per-domain "display_id"?
>>>
>>> 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?

Reinette