Messages in this thread Patch in this message | | | From | "Luck, Tony" <> | Subject | RE: [PATCH v17 7/9] x86/resctrl: Add new monitor files for Sub-NUMA cluster (SNC) monitoring | Date | Tue, 14 May 2024 18:26:09 +0000 |
| |
> 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?
> > >>>>> } > >>>>> + > >>>>> + 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 * @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
| |