Re: [PATCH v8 19/25] x86/resctrl: Auto assign/unassign counters when mbm_cntr_assign is enabled

[Reinette Chatre]

Hi Babu,

On 10/15/24 8:43 AM, Moger, Babu wrote:
> Hi Reinette/Tony,
> 
> On 10/14/24 21:39,  wrote:
>> Hi Babu,
>>
>> On 10/14/24 9:35 AM, Moger, Babu wrote:
>>> On 12/31/69 18:00, Luck, Tony wrote:
>>  
>>>>
>>>> It is still the case that callers don't care about the return value.
>>>
>>> That is correct.
>>>
>>
>> Are you planning to change this? I think Tony has a good point that since
>> assignment failures do not matter it unnecessarily complicates the code to
>> have rdtgroup_assign_cntrs() return failure.
>>
>> I also think the internals of rdtgroup_assign_cntrs() deserve a closer look.
>> I assume that error handling within rdtgroup_assign_cntrs() was created with
>> ABMC in mind. When only considering ABMC then the only reason why
>> rdtgroup_assign_cntr_event() could fail is if the system ran out of counters
>> and then indeed it makes no sense to attempt another call to rdtgroup_assign_cntr_event().
>>
>> Now that the resctrl fs/arch split is clear the implementation does indeed expose
>> another opportunity for failure ... if the arch callback, resctrl_arch_config_cntr()
>> fails. It could thus be possible for the first rdtgroup_assign_cntr_event() to fail
>> while the second succeeds. Earlier [1], Tony suggested to, within rdtgroup_assign_cntrs(),
>> remove the local ret variable and have it return void. This sounds good to me.
>> When doing so a function comment explaining the usage will be helpful.
>>
>> I also think that rdtgroup_unassign_cntrs() deserves similar scrutiny. Even more
>> so since I do not think that the second rdtgroup_unassign_cntr_event()
>> should be prevented from running if the first rdtgroup_unassign_cntr_event() fails.
> 
> 
> Sounds fine with me. Now it will look like this below.

Thank you for considering.

> 
> 

I assume that you will keep rdtgroup_assign_cntrs() function comment? I think
it may need some small changes to go with the function now returning void ...
for example, saying "Each group *requires* two counters" and then not failing when
two counters cannot be allocated seems suspect.

For example (please feel free to improve):

	Called when a new group is created. If "mbm_cntr_assign" mode is enabled,   
	counters are automatically assigned. Each group can accommodate two counters:      
	one for the total event and one for the local event. Assignments may fail
	due to the limited number of counters. However, it is not necessary to
	fail the group creation and thus no failure is returned. Users have the
	option to modify the counter assignments after the group has been created.   

> static void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
> {
>   struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> 
>  if (!resctrl_arch_mbm_cntr_assign_enabled(r))
>       return;
> 
>  if (is_mbm_total_enabled())
>    rdtgroup_assign_cntr_event(r, rdtgrp, NULL, QOS_L3_MBM_TOTAL_EVENT_ID);
> 
>  if (is_mbm_local_enabled())
>    rdtgroup_assign_cntr_event(r, rdtgrp, NULL, QOS_L3_MBM_LOCAL_EVENT_ID);
> 
> }
> 
> /*
>  * Called when a group is deleted. Counters are unassigned if it was in
>  * assigned state.
>  */
> static void rdtgroup_unassign_cntrs(struct rdtgroup *rdtgrp)
> {
>   struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> 
>   if (!resctrl_arch_mbm_cntr_assign_enabled(r))
>        return;
> 
>  if (is_mbm_total_enabled())
>  rdtgroup_unassign_cntr_event(r, rdtgrp, NULL, QOS_L3_MBM_TOTAL_EVENT_ID);
> 
>  if (is_mbm_local_enabled())
>  rdtgroup_unassign_cntr_event(r, rdtgrp, NULL, QOS_L3_MBM_LOCAL_EVENT_ID);
> 
> }

Looks good to me, thank you.

Reinette