* Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters [not found] ` <b1d3a653-79a5-44db-8d57-c052ae537823@intel.com> @ 2026-05-06 7:17 ` wuyifan 2026-05-07 16:13 ` Reinette Chatre 0 siblings, 1 reply; 3+ messages in thread From: wuyifan @ 2026-05-06 7:17 UTC (permalink / raw) To: Reinette Chatre, tony.luck, Dave.Martin, james.morse, babu.moger, shuah, tan.shaopeng, fenghuay, ben.horgan, jonathan.cameron, zengheng4, linux-kernel, linux-arm-kernel, linux-kselftest, linuxarm Cc: xiaqinxin, prime.zeng, wangyushan12, xuwei5, fanghao11, wangzhou1 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters 2026-05-06 7:17 ` [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters wuyifan @ 2026-05-07 16:13 ` Reinette Chatre 2026-05-08 10:14 ` wuyifan 0 siblings, 1 reply; 3+ messages in thread From: Reinette Chatre @ 2026-05-07 16:13 UTC (permalink / raw) To: wuyifan, tony.luck, Dave.Martin, james.morse, babu.moger, shuah, tan.shaopeng, fenghuay, ben.horgan, jonathan.cameron, zengheng4, linux-kernel, linux-arm-kernel, linux-kselftest, linuxarm Cc: xiaqinxin, prime.zeng, wangyushan12, xuwei5, fanghao11, wangzhou1 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters 2026-05-07 16:13 ` Reinette Chatre @ 2026-05-08 10:14 ` wuyifan 0 siblings, 0 replies; 3+ messages in thread From: wuyifan @ 2026-05-08 10:14 UTC (permalink / raw) To: Reinette Chatre, tony.luck, Dave.Martin, james.morse, babu.moger, shuah, tan.shaopeng, fenghuay, ben.horgan, jonathan.cameron, zengheng4, linux-kernel, linux-arm-kernel, linux-kselftest, linuxarm Cc: xiaqinxin, prime.zeng, wangyushan12, xuwei5, fanghao11, wangzhou1 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-08 10:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260410093352.3988125-1-wuyifan50@huawei.com>
[not found] ` <20260410093352.3988125-2-wuyifan50@huawei.com>
[not found] ` <b1d3a653-79a5-44db-8d57-c052ae537823@intel.com>
2026-05-06 7:17 ` [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters wuyifan
2026-05-07 16:13 ` Reinette Chatre
2026-05-08 10:14 ` wuyifan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox