Hi Reinette, On 10/15/24 12:18, wrote: > 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. > Looks good. Thanks -- Thanks Babu Moger
