On Mon, Nov 13, 2023 at 12:08:19PM -0600, Moger, Babu wrote:
> Hi Tony,
>
> Sorry for the late review. The patches look good for the most part. But we
> can simplify a little more. Please see my comments below.
>
>
> On 11/9/23 17:09, Tony Luck wrote:
> > The rdt_domain structure is used for both control and monitor features.
> > It is about to be split into separate structures for these two usages
> > because the scope for control and monitoring features for a resource
> > will be different for future resources.
> >
> > To allow for common code that scans a list of domains looking for a
> > specific domain id, move all the common fields ("list", "id", "cpu_mask")
> > into their own structure within the rdt_domain structure.
> >
> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> > include/linux/resctrl.h | 16 ++++--
> > arch/x86/kernel/cpu/resctrl/core.c | 26 +++++-----
> > arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 22 ++++-----
> > arch/x86/kernel/cpu/resctrl/monitor.c | 10 ++--
> > arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 14 +++---
> > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 60 +++++++++++------------
> > 6 files changed, 78 insertions(+), 70 deletions(-)
> >
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 7d4eb7df611d..c4067150a6b7 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -53,10 +53,20 @@ struct resctrl_staged_config {
> > };
> >
> > /**
> > - * struct rdt_domain - group of CPUs sharing a resctrl resource
> > + * struct rdt_domain_hdr - common header for different domain types
> > * @list: all instances of this resource
> > * @id: unique id for this instance
> > * @cpu_mask: which CPUs share this resource
> > + */
> > +struct rdt_domain_hdr {
> > + struct list_head list;
> > + int id;
> > + struct cpumask cpu_mask;
> > +};
>
> I like the idea of separating the domains, one for control and another for
> monitor. I have some comments on how it can be done to simplify the code.
> Adding the hdr adds a little complexity to the code.
>
> How about converting the current rdt_domain to explicitly to rdt_mon_domain?
>
> Something like this.
>
> struct rdt_mon_domain {
> struct list_head list;
> int id;
> struct cpumask cpu_mask;
> unsigned long *rmid_busy_llc;
> struct mbm_state *mbm_total;
> struct mbm_state *mbm_local;
> struct delayed_work mbm_over;
> struct delayed_work cqm_limbo;
> int mbm_work_cpu;
> int cqm_work_cpu;
> };
>
>
> Then introduce rdt_ctrl_domain to which separates into two doamins.
>
> struct rdt_ctrl_domain {
> struct list_head list;
> int id;
> struct cpumask cpu_mask;
> struct pseudo_lock_region *plr;
> struct resctrl_staged_config staged_config[CDP_NUM_TYPES];
> u32 *mbps_val;
> };
>
> I feel this will be easy to understand and makes the code simpler. Changes
> will be minimal.
Babu,
I went in this direction because of rdt_find_domain() which is used
to search either of the "ctrl" or "mon" lists. So it needs to be sure
that the "list" and "id" fields are at identical offsets in both the
rdt_ctrl_domain and rdt_mon_domain structures. Best way to guarantee[1]
that is to create the "hdr" entry (which later acquired the cpu_mask
field as a common element after a comment from Reinette).
One way to avoid this would be to essentially duplicate
rdt_find_domain() into rdt_find_ctrl_domain() and rdt_find_mon_domain()
... but I fear the function is just big enough to get complaints
that the two copies should somehow be merged.
-Tony
[1] In v5 I tried using a comment in each to say they must be the same,
but Reinette didn't like comments within declarations:
https://lore.kernel.org/all/5f1256d3-737e-a447-abbe-f541767b2c8f@xxxxxxxxx/
