Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters

Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters

[wuyifan]

Hi Reinette,

On 4/23/2026 12:02 AM, Reinette Chatre wrote:
> Hi Yifan,
>
> On 4/10/26 2:33 AM, Yifan Wu wrote:
>> @@ -113,6 +115,7 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>>   				    unsigned int *count)
>>   {
>>   	char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
>> +	struct imc_counter_config *imc_counter;
>>   	unsigned int orig_count = *count;
>>   	char cas_count_cfg[1024];
>>   	struct dirent *ep;
>> @@ -167,11 +170,17 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>>   			ksft_print_msg("Maximum iMC count exceeded\n");
>>   			goto out_close;
>>   		}
>> +		imc_counter = calloc(1, sizeof(*imc_counter));
>> +		if (!imc_counter) {
>> +			ksft_perror("Unable to allocate memory for iMC counters\n");
>> +			goto out_close;
>> +		}
>>   
>>   		imc_counters_config[*count].type = type;
>>   		get_read_event_and_umask(cas_count_cfg, *count);
>>   		/* Do not fail after incrementing *count. */
>>   		*count += 1;
>> +		list_add(&imc_counter->entry, &imc_counters_list);
>>   	}
>>   	if (*count == orig_count) {
>>   		ksft_print_msg("Unable to find events in %s\n", imc_events_dir);
> Should cleanup_read_mem_bw_imc() be called on error exit path?
Thank you for your suggestion. When parse_imc_read_bw_events() exits with an
error, the linked list imc_counters_list will be cleaned up in 
test_cleanup().

main()
└── run_single_test()
     ├── mbm_run_test()
     │   └── resctrl_val()
     │       └── mbm_init()
     │           └── initialize_read_mem_bw_imc()
     │               └── enumerate_imcs()
     │                   └── read_from_imc_dir()
     │                       └── parse_imc_read_bw_events()
     │                           └── calloc()
     └── test_cleanup()
         └── mbm_test_cleanup()
             └── cleanup_read_mem_bw_imc()

Calling cleanup_read_mem_bw_imc() in the error exit path may be intended
to prevent resource leaks. However, this results in the function being 
called
repeatedly in both the error exit branch and test_cleanup().

Is there any specific intention behind calling it in 
parse_imc_read_bw_events()?
Or should the cleanup be uniformly handled in test_cleanup()?

Yifan

Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters

[Reinette Chatre]

Hi Yifan,

On 5/6/26 12:17 AM, wuyifan wrote:
> Hi Reinette,
> 
> On 4/23/2026 12:02 AM, Reinette Chatre wrote:
>> Hi Yifan,
>>
>> On 4/10/26 2:33 AM, Yifan Wu wrote:
>>> @@ -113,6 +115,7 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>>>                       unsigned int *count)
>>>   {
>>>       char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
>>> +    struct imc_counter_config *imc_counter;
>>>       unsigned int orig_count = *count;
>>>       char cas_count_cfg[1024];
>>>       struct dirent *ep;
>>> @@ -167,11 +170,17 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>>>               ksft_print_msg("Maximum iMC count exceeded\n");
>>>               goto out_close;
>>>           }
>>> +        imc_counter = calloc(1, sizeof(*imc_counter));
>>> +        if (!imc_counter) {
>>> +            ksft_perror("Unable to allocate memory for iMC counters\n");
>>> +            goto out_close;
>>> +        }
>>>             imc_counters_config[*count].type = type;
>>>           get_read_event_and_umask(cas_count_cfg, *count);
>>>           /* Do not fail after incrementing *count. */
>>>           *count += 1;
>>> +        list_add(&imc_counter->entry, &imc_counters_list);
>>>       }
>>>       if (*count == orig_count) {
>>>           ksft_print_msg("Unable to find events in %s\n", imc_events_dir);
>> Should cleanup_read_mem_bw_imc() be called on error exit path?
> Thank you for your suggestion. When parse_imc_read_bw_events() exits with an
> error, the linked list imc_counters_list will be cleaned up in test_cleanup().
> 
> main()
> └── run_single_test()
>     ├── mbm_run_test()
>     │   └── resctrl_val()
>     │       └── mbm_init()
>     │           └── initialize_read_mem_bw_imc()
>     │               └── enumerate_imcs()
>     │                   └── read_from_imc_dir()
>     │                       └── parse_imc_read_bw_events()
>     │                           └── calloc()
>     └── test_cleanup()
>         └── mbm_test_cleanup()
>             └── cleanup_read_mem_bw_imc()
> 
> Calling cleanup_read_mem_bw_imc() in the error exit path may be intended
> to prevent resource leaks. However, this results in the function being called
> repeatedly in both the error exit branch and test_cleanup().

You are correct and calling it repeatedly is ok. When cleanup_read_mem_bw_imc() is
called from test_cleanup() after a failure in parse_imc_read_bw_events() then it
will find that the list is empty and just be a no-op. This is safe.

> 
> Is there any specific intention behind calling it in parse_imc_read_bw_events()?

The motivation behind calling it in parse_imc_read_bw_events() is to not leave this
memory allocated when this function fails. A function having a single responsibility
is easier to use and maintain since a caller does not need to take into account that
when the function fails it also needs to have additional responsibility to clean up
the state left behind by it. 

There may be some patterns where caller needs to clean up after a failure but that is
usually done in an obvious way where the caller _immediately_ does the cleanup on failure
but here this dependency is well hidden in this implementation with test_cleanup() being
called so far from parse_imc_read_bw_events(). This hidden dependency makes this code
difficult to use and maintain.

> Or should the cleanup be uniformly handled in test_cleanup()?

Handling it only in test_cleanup() may work in current execution flow but if the code is
ever re-factored this would result in a memory leak. It is not custom that callers need
to clean up state when a function fails and since this allocation is buried deep within the
execution flow I see this as a latent bug just waiting to be triggered.

Reinette

Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters

[wuyifan]

Hi Reinette

On 5/8/2026 12:13 AM, Reinette Chatre wrote:
>> Calling cleanup_read_mem_bw_imc() in the error exit path may be intended
>> to prevent resource leaks. However, this results in the function being called
>> repeatedly in both the error exit branch and test_cleanup().
> You are correct and calling it repeatedly is ok. When cleanup_read_mem_bw_imc() is
> called from test_cleanup() after a failure in parse_imc_read_bw_events() then it
> will find that the list is empty and just be a no-op. This is safe.
>
>> Is there any specific intention behind calling it in parse_imc_read_bw_events()?
> The motivation behind calling it in parse_imc_read_bw_events() is to not leave this
> memory allocated when this function fails. A function having a single responsibility
> is easier to use and maintain since a caller does not need to take into account that
> when the function fails it also needs to have additional responsibility to clean up
> the state left behind by it.
>
> There may be some patterns where caller needs to clean up after a failure but that is
> usually done in an obvious way where the caller_immediately_ does the cleanup on failure
> but here this dependency is well hidden in this implementation with test_cleanup() being
> called so far from parse_imc_read_bw_events(). This hidden dependency makes this code
> difficult to use and maintain.
>
>> Or should the cleanup be uniformly handled in test_cleanup()?
> Handling it only in test_cleanup() may work in current execution flow but if the code is
> ever re-factored this would result in a memory leak. It is not custom that callers need
> to clean up state when a function fails and since this allocation is buried deep within the
> execution flow I see this as a latent bug just waiting to be triggered.

Thank you for the detailed explanation. I understand the rationale now.

I will explicitly add cleanup calls in the error exit path in the next 
version.

Best regards,

Yifan