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

[Reinette Chatre]

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> ..."

> 
>>
>>>>>>>         }
>>>>>>> +
>>>>>>> +       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

Reinette