mbox series

[v2,0/9] selftests/resctrl: Fixes to error handling logic and cleanups

Message ID 20230215130605.31583-1-ilpo.jarvinen@linux.intel.com
Headers show
Series selftests/resctrl: Fixes to error handling logic and cleanups | expand

Message

Ilpo Järvinen Feb. 15, 2023, 1:05 p.m. UTC
This series fixes a few cleanup/error handling problems and cleans up
code.

v2:
- Improved changelogs
- Return NULL directly from malloc_and_init_memory()
- Added patch to convert memalign() to posix_memalign()
- Added patch to correct function comment parameter
- Dropped literal -> define patch for now (likely superceded soon)

Fenghua Yu (1):
  selftests/resctrl: Change name from CBM_MASK_PATH to INFO_PATH

Ilpo Järvinen (8):
  selftests/resctrl: Return NULL if malloc_and_init_memory() did not
    alloc mem
  selftests/resctrl: Move ->setup() call outside of test specific
    branches
  selftests/resctrl: Allow ->setup() to return errors
  selftests/resctrl: Check for return value after write_schemata()
  selftests/resctrl: Replace obsolete memalign() with posix_memalign()
  selftests/resctrl: Change initialize_llc_perf() return type to void
  selftests/resctrl: Use remount_resctrlfs() consistently with boolean
  selftests/resctrl: Correct get_llc_perf() param in function comment

 tools/testing/selftests/resctrl/cache.c       | 17 +++++++--------
 tools/testing/selftests/resctrl/cat_test.c    |  4 ++--
 tools/testing/selftests/resctrl/cmt_test.c    |  9 ++++----
 tools/testing/selftests/resctrl/fill_buf.c    |  7 +++++--
 tools/testing/selftests/resctrl/mba_test.c    | 11 +++++++---
 tools/testing/selftests/resctrl/mbm_test.c    |  4 ++--
 tools/testing/selftests/resctrl/resctrl.h     |  6 ++++--
 tools/testing/selftests/resctrl/resctrl_val.c | 21 +++++++------------
 tools/testing/selftests/resctrl/resctrlfs.c   |  2 +-
 9 files changed, 41 insertions(+), 40 deletions(-)

Comments

Reinette Chatre March 15, 2023, 11:59 p.m. UTC | #1
Hi Ilpo,

On 2/15/2023 5:05 AM, Ilpo Järvinen wrote:
> malloc_and_init_memory() in fill_buf isn't checking if memalign()
> successfully allocated memory or not before accessing the memory.
> 
> Check the return value of memalign() and return NULL if allocating
> aligned memory fails.
> 
> Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark")
> Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette
Reinette Chatre March 16, 2023, 12:02 a.m. UTC | #2
Hi Ilpo,

On 2/15/2023 5:05 AM, Ilpo Järvinen wrote:
> resctrl_val() assumes ->setup() always returns either 0 to continue
> tests or < 0 in case of the normal termination of tests after x runs.
> The latter overlaps with normal error returns.
> 
> Define END_OF_TESTS (=1) to differentiate the normal termination of
> tests and return errors as negative values. Alter callers of ->setup()
> to handle errors properly.
> 
> Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
> Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette
Reinette Chatre March 16, 2023, 12:02 a.m. UTC | #3
Hi Ilpo,

On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
> memalign() is obsolete according to its manpage.
> 
> Replace memalign() with posix_memalign() and remove malloc.h include
> that was there for memalign().
> 
> As a pointer is passed into posix_memalign(), initialize *p to NULL
> to silence a warning about the function's return value being used as
> uninitialized (which is not valid anyway because the error is properly
> checked before p is returned).
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette
Reinette Chatre March 16, 2023, 12:03 a.m. UTC | #4
Hi Ilpo,

On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> CBM_MASK_PATH is actually the path to resctrl/info.
> 
> Change the macro name to correctly indicate what it represents.
> 
> [ ij: Tweaked the changelog. ]
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette
Reinette Chatre March 16, 2023, 12:04 a.m. UTC | #5
Hi Ilpo,

On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
> get_llc_perf() function comment refers to cpu_no parameter that does
> not exist.
> 
> Correct get_llc_perf() the comment to document llc_perf_miss instead.

"Correct the get_llc_perf() comment"? This is so minor and I do not think
a reason to resubmit whole series.

> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette
Reinette Chatre March 16, 2023, 4:01 p.m. UTC | #6
Hi Ilpo,

On 2/15/2023 5:05 AM, Ilpo Järvinen wrote:
> This series fixes a few cleanup/error handling problems and cleans up
> code.
> 

Thank you very much. These are great cleanups. Looks like I missed
sending one response with the others but at this time all patches
in this series should have my Reviewed-by tag.

If the Kselftest team finds them acceptable I hope that they
can help to route them upstream.

Reinette