lkml.org 
[lkml]   [2024]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRE: [PATCH v17 7/9] x86/resctrl: Add new monitor files for Sub-NUMA cluster (SNC) monitoring
Date
> 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
\
 
 \ /
  Last update: 2024-05-27 18:26    [W:0.097 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site