Re: [PATCH v4 3/7] x86/resctrl: Change monitor code to use rdt_mondomain

[Tony Luck <tony.luck@intel.com>]

On Fri, Aug 11, 2023 at 10:30:20AM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 7/22/2023 12:07 PM, Tony Luck wrote:
> > A few functions need to be duplicated to provide versions to
> > operate on control and monitor domains respectively. But most
> > of the changes are just fixing argument and return value types.
> 
> Could you please add some context in support of this change?
> 
> I do not think "duplicated" is appropriate though. Functions
> are not duplicated but instead made to be dedicated to
> either control or monitoring domains, no?

Commit comment rewritten based on these suggestions

> 
> ...
> 
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 274605aaa026..0161362b0c3e 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -393,9 +393,12 @@ void rdt_ctrl_update(void *arg)
> >   * id is found in a domain, return the domain. Otherwise, if requested by
> >   * caller, return the first domain whose id is bigger than the input id.
> >   * The domain list is sorted by id in ascending order.
> > + *
> > + * N.B. Returned value may be either a pointer to "struct rdt_domain" or
> > + * to "struct rdt_mondomain" depending on which domain list is scanned.
> >   */
> > -struct rdt_domain *rdt_find_domain(struct list_head *h, int id,
> > -				   struct list_head **pos)
> > +void *rdt_find_domain(struct list_head *h, int id,
> > +		      struct list_head **pos)
> >  {
> >  	struct rdt_domain *d;
> >  	struct list_head *l;
> 
> I do not think that void pointers should be passed around. How about two
> new functions dedicated to the different domain types with the void pointer
> handling contained in a static function? For example,
> 
> static void *__rdt_find_domain(struct list_head *h, int id, struct list_head **pos)
> 
> struct rdt_mondomain *rdt_find_mondomain(struct rdt_resource *r, int id, struct list_head **pos)
> struct rdt_domain *rdt_find_ctrldomain(struct rdt_resource *r, int id, struct list_head **pos)
> 
> rdt_find_mondomain() and rdt_find_ctrldomain() would be what callers use
> while they can be wrappers of __rdt_find_domain().

Good suggestion. Initial bits are in patch 1. Types changed later
after struct rdt_mondomain is added.

> 
> 
> > @@ -434,10 +437,15 @@ static void setup_default_ctrlval(struct rdt_resource *r, u32 *dc)
> >  }
> >  
> >  static void domain_free(struct rdt_hw_domain *hw_dom)
> > +{
> > +	kfree(hw_dom->ctrl_val);
> > +	kfree(hw_dom);
> > +}
> > +
> > +static void mondomain_free(struct rdt_hw_mondomain *hw_dom)
> >  {
> >  	kfree(hw_dom->arch_mbm_total);
> >  	kfree(hw_dom->arch_mbm_local);
> > -	kfree(hw_dom->ctrl_val);
> >  	kfree(hw_dom);
> >  }
> >  
> > @@ -467,7 +475,7 @@ static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
> >   * @num_rmid:	The size of the MBM counter array
> >   * @hw_dom:	The domain that owns the allocated arrays
> >   */
> > -static int arch_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_domain *hw_dom)
> > +static int arch_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_mondomain *hw_dom)
> >  {
> >  	size_t tsize;
> >  
> > @@ -539,8 +547,8 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> >  {
> >  	int id = get_cpu_cacheinfo_id(cpu, r->mon_scope);
> >  	struct list_head *add_pos = NULL;
> > -	struct rdt_hw_domain *hw_dom;
> > -	struct rdt_domain *d;
> > +	struct rdt_hw_mondomain *hw_mondom;
> > +	struct rdt_mondomain *d;
> >  	int err;
> >  
> 
> Please ensure that reverse fir tree order is maintained in all these changes.

Oops. Fixed this one and looked around for any others introduced by me.
> 
> Reinette

-Tony