[PATCH 0/4] selftests/resctrl: Fixes to error handling logic

[PATCH 0/4] selftests/resctrl: Fixes to error handling logic

[Ilpo Järvinen]

This series fixes a few cleanup/error handling problems.

Fenghua Yu (1):
  selftests/resctrl: Return error if memory is not allocated

Ilpo Järvinen (3):
  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()

 tools/testing/selftests/resctrl/cache.c       |  4 +++-
 tools/testing/selftests/resctrl/cat_test.c    |  2 +-
 tools/testing/selftests/resctrl/cmt_test.c    |  2 +-
 tools/testing/selftests/resctrl/fill_buf.c    |  2 ++
 tools/testing/selftests/resctrl/mba_test.c    |  9 ++++++--
 tools/testing/selftests/resctrl/mbm_test.c    |  2 +-
 tools/testing/selftests/resctrl/resctrl.h     |  2 ++
 tools/testing/selftests/resctrl/resctrl_val.c | 21 +++++++------------
 8 files changed, 25 insertions(+), 19 deletions(-)

-- 
2.30.2

[PATCH 1/4] selftests/resctrl: Return error if memory is not allocated

[Ilpo Järvinen]

From: Fenghua Yu <fenghua.yu@intel.com>

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")
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 tools/testing/selftests/resctrl/fill_buf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 56ccbeae0638..f4880c962ec4 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s)
 	size_t s64;
 
 	void *p = memalign(PAGE_SIZE, s);
+	if (!p)
+		return p;
 
 	p64 = (uint64_t *)p;
 	s64 = s / sizeof(uint64_t);
-- 
2.30.2

Re: [PATCH 1/4] selftests/resctrl: Return error if memory is not allocated

[Reinette Chatre]

Hi Ilpo,

I do not see a why two patch series are needed for
the resctrl fixes. It may make it easier for everybody if
it is handled as one patch series (with fixes first)?

On 2/8/2023 1:30 AM, Ilpo Järvinen wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> 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")
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Missing your Signed-off-by?

> ---
>  tools/testing/selftests/resctrl/fill_buf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 56ccbeae0638..f4880c962ec4 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s)
>  	size_t s64;
>  
>  	void *p = memalign(PAGE_SIZE, s);

This may also be a good time to stop using an obsolete call?

> +	if (!p)
> +		return p;

Could you please return NULL explicitly?

>  
>  	p64 = (uint64_t *)p;
>  	s64 = s / sizeof(uint64_t);


Reinette

Re: [PATCH 1/4] selftests/resctrl: Return error if memory is not allocated

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

On Mon, 13 Feb 2023, Reinette Chatre wrote:

> I do not see a why two patch series are needed for
> the resctrl fixes. It may make it easier for everybody if
> it is handled as one patch series (with fixes first)?

Ok, I can put the fixes and cleanups into one series.

> On 2/8/2023 1:30 AM, Ilpo Järvinen wrote:
> > From: Fenghua Yu <fenghua.yu@intel.com>
> > 
> > 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")
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> 
> Missing your Signed-off-by?

These were intentionally. When I didn't modify the original patch at 
all during forward porting it, I just kept the original From and SoB as 
is. But from the doc you pointed me to, I see now x86 wants also handlers 
sobs.

> > ---
> >  tools/testing/selftests/resctrl/fill_buf.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> > index 56ccbeae0638..f4880c962ec4 100644
> > --- a/tools/testing/selftests/resctrl/fill_buf.c
> > +++ b/tools/testing/selftests/resctrl/fill_buf.c
> > @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s)
> >  	size_t s64;
> >  
> >  	void *p = memalign(PAGE_SIZE, s);
> 
> This may also be a good time to stop using an obsolete call?

Sure, I can add another patch to change that to posix_memalign().

> > +	if (!p)
> > +		return p;
> 
> Could you please return NULL explicitly?

I'll change it.

Thanks for you comments.

-- 
 i.

Re: [PATCH 1/4] selftests/resctrl: Return error if memory is not allocated

[Reinette Chatre]

Hi Ilpo,

On 2/14/2023 1:32 AM, Ilpo Järvinen wrote:
> On Mon, 13 Feb 2023, Reinette Chatre wrote:

>> Missing your Signed-off-by?
> 
> These were intentionally. When I didn't modify the original patch at 
> all during forward porting it, I just kept the original From and SoB as 
> is. But from the doc you pointed me to, I see now x86 wants also handlers 
> sobs.

I do not think this is x86 specific. 
Documentation/process/submitting-patches.rst states:
"Any further SoBs (Signed-off-by:'s) following the author's SoB are from
people handling and transporting the patch, but were not involved in its
development. SoB chains should reflect the **real** route a patch took
as it was propagated to the maintainers and ultimately to Linus, with
the first SoB entry signalling primary authorship of a single author."


> 
>>> ---
>>>  tools/testing/selftests/resctrl/fill_buf.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
>>> index 56ccbeae0638..f4880c962ec4 100644
>>> --- a/tools/testing/selftests/resctrl/fill_buf.c
>>> +++ b/tools/testing/selftests/resctrl/fill_buf.c
>>> @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s)
>>>  	size_t s64;
>>>  
>>>  	void *p = memalign(PAGE_SIZE, s);
>>
>> This may also be a good time to stop using an obsolete call?
> 
> Sure, I can add another patch to change that to posix_memalign().

You can also consider aligned_alloc().

Reinette

[PATCH 2/4] selftests/resctrl: Move ->setup() call outside of test specific branches

[Ilpo Järvinen]

resctrl_val() function is called only by mbm, mba, and cmt tests which
means the else branch is never used and can be removed.

Presently, both of the the test branches call param->setup(). Thus, the
call can be placed outside of the test specific branches reducing code
duplication.

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>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index b32b96356ec7..787546a52849 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -734,29 +734,22 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 
 	/* Test runs until the callback setup() tells the test to stop. */
 	while (1) {
+		ret = param->setup(1, param);
+		if (ret) {
+			ret = 0;
+			break;
+		}
+
 		if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
 		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-			ret = param->setup(1, param);
-			if (ret) {
-				ret = 0;
-				break;
-			}
-
 			ret = measure_vals(param, &bw_resc_start);
 			if (ret)
 				break;
 		} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
-			ret = param->setup(1, param);
-			if (ret) {
-				ret = 0;
-				break;
-			}
 			sleep(1);
 			ret = measure_cache_vals(param, bm_pid);
 			if (ret)
 				break;
-		} else {
-			break;
 		}
 	}
 
-- 
2.30.2

[PATCH 3/4] selftests/resctrl: Allow ->setup() to return errors

[Ilpo Järvinen]

Currently 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>
---
 tools/testing/selftests/resctrl/cache.c       | 4 +++-
 tools/testing/selftests/resctrl/cat_test.c    | 2 +-
 tools/testing/selftests/resctrl/cmt_test.c    | 2 +-
 tools/testing/selftests/resctrl/mba_test.c    | 2 +-
 tools/testing/selftests/resctrl/mbm_test.c    | 2 +-
 tools/testing/selftests/resctrl/resctrl.h     | 2 ++
 tools/testing/selftests/resctrl/resctrl_val.c | 4 +++-
 7 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 68ff856d36f0..0485863a169f 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -244,10 +244,12 @@ int cat_val(struct resctrl_val_param *param)
 	while (1) {
 		if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
 			ret = param->setup(1, param);
-			if (ret) {
+			if (ret == END_OF_TESTS) {
 				ret = 0;
 				break;
 			}
+			if (ret < 0)
+				break;
 			ret = reset_enable_llc_perf(bm_pid, param->cpu_no);
 			if (ret)
 				break;
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 1c5e90c63254..2d3c7c77ab6c 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -40,7 +40,7 @@ static int cat_setup(int num, ...)
 
 	/* Run NUM_OF_RUNS times */
 	if (p->num_of_runs >= NUM_OF_RUNS)
-		return -1;
+		return END_OF_TESTS;
 
 	if (p->num_of_runs == 0) {
 		sprintf(schemata, "%lx", p->mask);
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 8968e36db99d..3b0454e7fc82 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -32,7 +32,7 @@ static int cmt_setup(int num, ...)
 
 	/* Run NUM_OF_RUNS times */
 	if (p->num_of_runs >= NUM_OF_RUNS)
-		return -1;
+		return END_OF_TESTS;
 
 	p->num_of_runs++;
 
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 1a1bdb6180cf..f32289ae17ae 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -41,7 +41,7 @@ static int mba_setup(int num, ...)
 		return 0;
 
 	if (allocation < ALLOCATION_MIN || allocation > ALLOCATION_MAX)
-		return -1;
+		return END_OF_TESTS;
 
 	sprintf(allocation_str, "%d", allocation);
 
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 8392e5c55ed0..280187628054 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -95,7 +95,7 @@ static int mbm_setup(int num, ...)
 
 	/* Run NUM_OF_RUNS times */
 	if (num_of_runs++ >= NUM_OF_RUNS)
-		return -1;
+		return END_OF_TESTS;
 
 	va_start(param, num);
 	p = va_arg(param, struct resctrl_val_param *);
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index f0ded31fb3c7..f44fa2de4d98 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -37,6 +37,8 @@
 #define ARCH_INTEL     1
 #define ARCH_AMD       2
 
+#define END_OF_TESTS	1
+
 #define PARENT_EXIT(err_msg)			\
 	do {					\
 		perror(err_msg);		\
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 787546a52849..00864242d76c 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -735,10 +735,12 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 	/* Test runs until the callback setup() tells the test to stop. */
 	while (1) {
 		ret = param->setup(1, param);
-		if (ret) {
+		if (ret == END_OF_TESTS) {
 			ret = 0;
 			break;
 		}
+		if (ret < 0)
+			break;
 
 		if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
 		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-- 
2.30.2

[PATCH 4/4] selftests/resctrl: Check for return value after write_schemata()

[Ilpo Järvinen]

MBA test case writes schemata but it forgets to check if the write is
successful or not. Hence, add the check and return error properly.

Fixes: 01fee6b4d1f9 ("selftests/resctrl: Add MBA test")
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>
---
 tools/testing/selftests/resctrl/mba_test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index f32289ae17ae..97dc98c0c949 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -28,6 +28,7 @@ static int mba_setup(int num, ...)
 	struct resctrl_val_param *p;
 	char allocation_str[64];
 	va_list param;
+	int ret;
 
 	va_start(param, num);
 	p = va_arg(param, struct resctrl_val_param *);
@@ -45,7 +46,11 @@ static int mba_setup(int num, ...)
 
 	sprintf(allocation_str, "%d", allocation);
 
-	write_schemata(p->ctrlgrp, allocation_str, p->cpu_no, p->resctrl_val);
+	ret = write_schemata(p->ctrlgrp, allocation_str, p->cpu_no,
+			     p->resctrl_val);
+	if (ret < 0)
+		return ret;
+
 	allocation -= ALLOCATION_STEP;
 
 	return 0;
-- 
2.30.2