Hi Reinette,
Sorry.. Was on a vacation. Addressing the comments now.
On 12/23/24 15:13, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/18/24 1:37 PM, Babu Moger wrote:
>> Introduce io_alloc_capable in struct resctrl_cache to detect SDCIAE
>> (L3 Smart Data Cache Injection Allocation Enforcement) feature.
>
> Please distinguish clearly between the resctrl feature ("io_alloc_capable")
> and the architecture specific feature (SDCIAE) that backs it. This is similar
> to what you have done for ABMC and makes the work much easier to understand.
> When the resctrl and arch feature is used interchangeably it becomes confusing.
Sure. Will rewrite the commit text.
>
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>> v2: Changed sdciae_capable to io_alloc_capable to make it generic feature.
>> Also moved the io_alloc_capable in struct resctrl_cache.
>> ---
>> arch/x86/kernel/cpu/resctrl/core.c | 7 +++++++
>> include/linux/resctrl.h | 4 ++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index c2450cd52511..39e110033d96 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -306,6 +306,11 @@ static void rdt_get_cdp_config(int level)
>> rdt_resources_all[level].r_resctrl.cdp_capable = true;
>> }
>>
>> +static void rdt_get_sdciae_alloc_cfg(struct rdt_resource *r)
>> +{
>> + r->cache.io_alloc_capable = true;
>> +}
>
> rdt_get_sdciae_alloc_cfg() looks like one that should have "set" in its name, not "get".
> This also does not seem architecture specific so "sdciae" should not be in the name.
> rdt_set_io_alloc_capable()?
Sure.
>
>> +
>> static void rdt_get_cdp_l3_config(void)
>> {
>> rdt_get_cdp_config(RDT_RESOURCE_L3);
>> @@ -931,6 +936,8 @@ static __init bool get_rdt_alloc_resources(void)
>> rdt_get_cache_alloc_cfg(1, r);
>> if (rdt_cpu_has(X86_FEATURE_CDP_L3))
>> rdt_get_cdp_l3_config();
>> + if (rdt_cpu_has(X86_FEATURE_SDCIAE))
>> + rdt_get_sdciae_alloc_cfg(r);
>> ret = true;
>> }
>> if (rdt_cpu_has(X86_FEATURE_CAT_L2)) {
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index d94abba1c716..5837acff7442 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -129,6 +129,8 @@ struct rdt_mon_domain {
>> * @arch_has_sparse_bitmasks: True if a bitmask like f00f is valid.
>> * @arch_has_per_cpu_cfg: True if QOS_CFG register for this cache
>> * level has CPU scope.
>> + * @io_alloc_capable: Smart Data Cache Injection Allocation Enforcement
>> + * capable (SDCIAE).
>
> Please remove arch specific text here. For example,
> "True if portion of the L3 cache can be allocated for I/O traffic."
Sure.
>
>> */
>> struct resctrl_cache {
>> unsigned int cbm_len;
>> @@ -136,6 +138,7 @@ struct resctrl_cache {
>> unsigned int shareable_bits;
>> bool arch_has_sparse_bitmasks;
>> bool arch_has_per_cpu_cfg;
>> + bool io_alloc_capable;
>> };
>>
>> /**
>> @@ -202,6 +205,7 @@ enum resctrl_scope {
>> * @evt_list: List of monitoring events
>> * @fflags: flags to choose base and info files
>> * @cdp_capable: Is the CDP feature available on this resource
>> + * @sdciae_capable: Is SDCIAE feature available on this resource
>> */
>> struct rdt_resource {
>> int rid;
>
> Leftover from previous version?
Yes. Will remove it.
Thanks
Babu Moger
