Re: [PATCH v6 10/24] x86/resctrl: Allocate the cleanest CLOSID by searching closid_num_dirty_rmid

[James Morse <james.morse@arm.com>]

Hi Babu,

On 05/10/2023 21:13, Moger, Babu wrote:
> On 9/14/2023 12:21 PM, James Morse wrote:
>> MPAM's PMG bits extend its PARTID space, meaning the same PMG value can be
>> used for different control groups.
>>
>> This means once a CLOSID is allocated, all its monitoring ids may still be
>> dirty, and held in limbo.
>>
>> Instead of allocating the first free CLOSID, on architectures where
>> CONFIG_RESCTRL_RMID_DEPENDS_ON_COSID is enabled, search
>> closid_num_dirty_rmid[] to find the cleanest CLOSID.
>>
>> The CLOSID found is returned to closid_alloc() for the free list
>> to be updated.

>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index ad6e874d9ed2..f06d3d3e0808 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -558,5 +558,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>   void __init thread_throttle_mode_init(void);
>>   void __init mbm_config_rftype_init(const char *config);
>>   void rdt_staged_configs_clear(void);
>> +bool closid_allocated(unsigned int closid);
>> +int resctrl_find_cleanest_closid(void);
>>     #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 0c783301d106..0bbed8c62d42 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -388,6 +388,48 @@ static struct rmid_entry *resctrl_find_free_rmid(u32 closid)
>>       return ERR_PTR(-ENOSPC);
>>   }
>>   +/**
>> + * resctrl_find_cleanest_closid() - Find a CLOSID where all the associated
>> + *                                  RMID are clean, or the CLOSID that has
>> + *                                  the most clean RMID.
>> + *
>> + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocated CLOSID
>> + * may not be able to allocate clean RMID. To avoid this the allocator will
>> + * choose the CLOSID with the most clean RMID.
>> + *
>> + * When the CLOSID and RMID are independent numbers, the first free CLOSID will
>> + * be returned.
>> + */
>> +int resctrl_find_cleanest_closid(void)
>> +{
>> +    u32 cleanest_closid = ~0, iter_num_dirty;
> 
> Just naming num_dirty should have been fine.  I will leave it you.

That was to make it obvious its something to do with the loop, so the value can't be
relied on outside that. I'll rename it and move the declaration into the loop, that way
its out of scope if someone tries to use it later.


>> +    int i = 0;
>> +
>> +    lockdep_assert_held(&rdtgroup_mutex);
>> +
>> +    if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>> +        return -EIO;
>> +
>> +    for (i = 0; i < closids_supported(); i++) {
>> +        if (closid_allocated(i))
>> +            continue;
>> +
>> +        iter_num_dirty = closid_num_dirty_rmid[i];
>> +        if (iter_num_dirty == 0)
>> +            return i;
>> +
>> +        if (cleanest_closid == ~0)
>> +            cleanest_closid = i;
>> +
>> +        if (iter_num_dirty < closid_num_dirty_rmid[cleanest_closid])
>> +            cleanest_closid = i;
>> +    }
>> +
>> +    if (cleanest_closid == ~0)
>> +        return -ENOSPC;
>> +    return cleanest_closid;
> 
> Line before the return looks clean.
> 
> *       if (cleanest_closid == ~0)
> +        return -ENOSPC;
> +
> +    return cleanest_closid;

Sure,


Thanks,

James