Message-ID: <SJ1PR11MB60838E2DB2984EF95D34C692FCE32@SJ1PR11MB6083.namprd11.prod.outlook.com>
Date: Tue, 14 May 2024 18:26:09 +0000
From: "Luck, Tony" <tony.luck@...el.com>
To: "Chatre, Reinette" <reinette.chatre@...el.com>
CC: "Yu, Fenghua" <fenghua.yu@...el.com>, "Wieczor-Retman, Maciej"
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>, "Drew
 Fustini" <dfustini@...libre.com>, Dave Martin <Dave.Martin@....com>,
	"x86@...nel.org" <x86@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "patches@...ts.linux.dev"
	<patches@...ts.linux.dev>
Subject: RE: [PATCH v17 7/9] x86/resctrl: Add new monitor files for Sub-NUMA
 cluster (SNC) monitoring

> 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

Powered by blists - more mailing lists