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
