RE: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Babu Moger]

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Sent: Thursday, May 14, 2020 1:11 PM
> To: Moger, Babu <Babu.Moger@xxxxxxx>; tglx@xxxxxxxxxxxxx;
> fenghua.yu@xxxxxxxxx; bp@xxxxxxxxx; tony.luck@xxxxxxxxx
> Cc: kuo-lang.tseng@xxxxxxxxx; ravi.v.shankar@xxxxxxxxx; mingo@xxxxxxxxxx;
> hpa@xxxxxxxxx; x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
> throttling mode
> 
> Hi Babu,
> 
> Thank you very much for taking a look.
> 
> On 5/14/2020 9:45 AM, Babu Moger wrote:
> > Hi Reinnette,
> >
> > The patches did not apply on my tree. I got the latest tree today. You
> 
> Which tree did you use as baseline? These patches should apply cleanly
> on top of the other resctrl patches already queued for inclusion into
> v5.8 as found on the x86/cache branch of the tip repo.

Oh. Ok. I was using linux master.  Never mind..

> 
> > might want to check again.
> > Hunk #1 FAILED at 29.
> > 1 out of 7 hunks FAILED -- saving rejects to file
> > arch/x86/kernel/cpu/resctrl/rdtgroup.c.rej
> >
> >
> >> -----Original Message-----
> >> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> >> Sent: Wednesday, May 6, 2020 6:50 PM
> >> To: tglx@xxxxxxxxxxxxx; fenghua.yu@xxxxxxxxx; bp@xxxxxxxxx;
> >> tony.luck@xxxxxxxxx
> >> Cc: kuo-lang.tseng@xxxxxxxxx; ravi.v.shankar@xxxxxxxxx;
> mingo@xxxxxxxxxx;
> >> Moger, Babu <Babu.Moger@xxxxxxx>; hpa@xxxxxxxxx; x86@xxxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx; Reinette Chatre <reinette.chatre@xxxxxxxxx>
> >> Subject: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
> >> throttling mode
> >>
> 
> ...
> 
> >> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
> >> b/arch/x86/kernel/cpu/resctrl/core.c
> >> index 12f967c6b603..1bc686777069 100644
> >> --- a/arch/x86/kernel/cpu/resctrl/core.c
> >> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> >> @@ -250,6 +250,30 @@ static inline bool rdt_get_mb_table(struct
> rdt_resource
> >> *r)
> >>  	return false;
> >>  }
> >>
> >> +/*
> >> + * Model-specific test to determine if platform where memory bandwidth
> >> + * control is applied to a core can be configured to apply either the
> >> + * maximum or minimum of the per-thread delay values.
> >> + * By default, platforms where memory bandwidth control is applied to a
> >> + * core will select the maximum delay value of the per-thread CLOS.
> >> + *
> >> + * NOTE: delay value programmed to hardware is inverse of bandwidth
> >> + * percentage configured via user interface.
> >> + */
> >> +bool mba_cfg_supports_min_max_intel(void)
> >> +{
> >> +	switch (boot_cpu_data.x86_model) {
> >> +	case INTEL_FAM6_ATOM_TREMONT_D:
> >> +	case INTEL_FAM6_ICELAKE_X:
> >> +	case INTEL_FAM6_ICELAKE_D:
> >> +		return true;
> >> +	default:
> >> +		return false;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >
> > I see that you are calling this function multiple times. Why don't you
> 
> It is called:
> - once during initialization
> - once per package initialization (when first CPU on a package comes online)
> - once per read from or write to the new "thread_throttle_mode" file
> 
> > make it as a property in rdt_resource. Set it only once during the
> > init(may be in get_mem_config_intel). Then you can use it wherever
> > required. This also probably help James to make everything architecture
> > independent. What do you think?
> 
> If I understand correctly this would require understanding how each
> architecture behaves in this regard to ensure the property within
> rdt_resource is initialized correctly. While it could just be set within
> get_mem_config_intel as you suggest, this property would be present in
> AMD's resource and thus need a value ... could you please provide
> guidance what this property should be on AMD? I looked at the bandwidth
> enforcement section of
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevelop
> er.amd.com%2Fwp-
> content%2Fresources%2F56375.pdf&amp;data=02%7C01%7Cbabu.moger%40a
> md.com%7C5e7d546489d54002870808d7f8322934%7C3dd8961fe4884e608e11
> a82d994e183d%7C0%7C0%7C637250766898923893&amp;sdata=hQ6usFmXiUX
> p%2BZCrM2qdVn2Z3Ggx1c2g4nVwrEqrzG4%3D&amp;reserved=0 but it is not
> obvious to me where bandwidth is actually enforced and how this
> enforcement is affected when threads and/or cores are running tasks with
> different CLOS that have different bandwidth limits assigned.
> 
> With AMD's properties understood then the new "thread_throttle_mode"
> file could be visible on all platforms, not just Intel, and display
> accurate values for all and be architecture independent.
> 
> Alternatively, the new property could have values: UNDEFINED, MIN, MAX,
> or PER_THREAD ... with AMD having UNDEFINED and the
> "thread_throttle_mode" continues to be visible on Intel platforms only.
> 
> I would appreciate your thoughts on this.

Here is the text from spec.
"Platform QOS features are implemented on a logical processor basis.
Therefore, multiple hardware threads of a single physical CPU core may
have independent resource monitoring and enforcement configurations."

So, bandwidth allocation is already at thread level. But AMD does not use
memory delay throttle model to control the allocation like Intel does. So,
I would say you can initialize it as UNDEFINED not to cause any confusion.


> 
> > I assume that this property is probably not part of CPUID.
> 
> Correct. This specific property is model specific, but also transient in
> that it is replaced by X86_FEATURE_PER_THREAD_MBA (that is part of
> CPUID) in future platforms.
> 
> Reinette