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
