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

[Reinette Chatre]

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?

...

> 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().


> @@ -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.

Reinette