diff mbox series

[v2,02/26] selftests/resctrl: Split fill_buf to allow tests finer-grained control

Message ID 20231120111340.7805-3-ilpo.jarvinen@linux.intel.com
State New
Headers
Series selftests/resctrl: CAT test improvements & generalized test framework |

Commit Message

Ilpo Järvinen Nov. 20, 2023, 11:13 a.m. UTC 
  MBM, MBA and CMT test cases call run_fill_buf() that in turn calls
fill_cache() to alloc and loop indefinitely around the buffer. This
binds buffer allocation and running the benchmark into a single bundle
so that a selftest cannot allocate a buffer once and reuse it. CAT test
doesn't want to loop around the buffer continuously and after rewrite
it needs the ability to allocate the buffer separately.

Split buffer allocation out of fill_cache() into alloc_buffer(). This
change is part of preparation for the new CAT test that allocates a
buffer and does multiple passes over the same buffer (but not in an
infinite loop).

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/fill_buf.c | 26 +++++++++++++---------
 1 file changed, 15 insertions(+), 11 deletions(-)

Comments

Reinette Chatre Nov. 28, 2023, 10:10 p.m. UTC | #1 
Hi Ilpo,

On 11/20/2023 3:13 AM, Ilpo Järvinen wrote:
> MBM, MBA and CMT test cases call run_fill_buf() that in turn calls
> fill_cache() to alloc and loop indefinitely around the buffer. This
> binds buffer allocation and running the benchmark into a single bundle
> so that a selftest cannot allocate a buffer once and reuse it. CAT test
> doesn't want to loop around the buffer continuously and after rewrite
> it needs the ability to allocate the buffer separately.
> 
> Split buffer allocation out of fill_cache() into alloc_buffer(). This
> change is part of preparation for the new CAT test that allocates a
> buffer and does multiple passes over the same buffer (but not in an
> infinite loop).
> 
> 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>
> ---

Could you please list the changes to a patch in this area instead of
lumping all in the cover letter? Without this it is not clear what,
if anything, changed in the new version.

>  tools/testing/selftests/resctrl/fill_buf.c | 26 +++++++++++++---------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 0d425f26583a..6f32f44128e1 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -135,33 +135,37 @@ static int fill_cache_write(unsigned char *buf, size_t buf_size, bool once)
>  	return 0;
>  }
>  
> -static int fill_cache(size_t buf_size, int memflush, int op, bool once)
> +static unsigned char *alloc_buffer(size_t buf_size, int memflush)
>  {
>  	unsigned char *buf;
> -	int ret;
>  
>  	buf = malloc_and_init_memory(buf_size);
>  	if (!buf)
> -		return -1;
> +		return NULL;
>  
>  	/* Flush the memory before using to avoid "cache hot pages" effect */
>  	if (memflush)
>  		mem_flush(buf, buf_size);
>  
> +	return buf;
> +}
> +
> +static int fill_cache(size_t buf_size, int memflush, int op, bool once)
> +{
> +	unsigned char *buf;
> +	int ret;
> +
> +	buf = alloc_buffer(buf_size, memflush);
> +	if (!buf)
> +		return -1;
> +
>  	if (op == 0)
>  		ret = fill_cache_read(buf, buf_size, once);
>  	else
>  		ret = fill_cache_write(buf, buf_size, once);
> -
>  	free(buf);
>  
> -	if (ret) {
> -		printf("\n Error in fill cache read/write...\n");
> -		return -1;
> -	}
> -

The changelog does not motivate the removal of this error message.
It seems ok at this point since the only failing functions already
print their own error message. Without a motivation of this change
it is not clear if it is actually intended.

In any case, this looks good.

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

Reinette
Ilpo Järvinen Dec. 7, 2023, 2:04 p.m. UTC | #2 
On Tue, 28 Nov 2023, Reinette Chatre wrote:
> On 11/20/2023 3:13 AM, Ilpo Järvinen wrote:
> > MBM, MBA and CMT test cases call run_fill_buf() that in turn calls
> > fill_cache() to alloc and loop indefinitely around the buffer. This
> > binds buffer allocation and running the benchmark into a single bundle
> > so that a selftest cannot allocate a buffer once and reuse it. CAT test
> > doesn't want to loop around the buffer continuously and after rewrite
> > it needs the ability to allocate the buffer separately.
> > 
> > Split buffer allocation out of fill_cache() into alloc_buffer(). This
> > change is part of preparation for the new CAT test that allocates a
> > buffer and does multiple passes over the same buffer (but not in an
> > infinite loop).
> > 
> > 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>
> > ---
> 
> Could you please list the changes to a patch in this area instead of
> lumping all in the cover letter? Without this it is not clear what,
> if anything, changed in the new version.

Okay, I'll try to keep them per patch.

> > +static int fill_cache(size_t buf_size, int memflush, int op, bool once)
> > +{
> > +	unsigned char *buf;
> > +	int ret;
> > +
> > +	buf = alloc_buffer(buf_size, memflush);
> > +	if (!buf)
> > +		return -1;
> > +
> >  	if (op == 0)
> >  		ret = fill_cache_read(buf, buf_size, once);
> >  	else
> >  		ret = fill_cache_write(buf, buf_size, once);
> > -
> >  	free(buf);
> >  
> > -	if (ret) {
> > -		printf("\n Error in fill cache read/write...\n");
> > -		return -1;
> > -	}
> > -
> 
> The changelog does not motivate the removal of this error message.
> It seems ok at this point since the only failing functions already
> print their own error message. Without a motivation of this change
> it is not clear if it is actually intended.

Hi,

I don't have recollection of how it ended up into this patch as it's so 
long time ago already... But in any case, it naturally seemed to fit into 
the next patch that removes the extra call level so I moved the removal 
of the duplicated error printout to that patch instead.

> In any case, this looks good.
> 
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
diff mbox series

Patch

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 0d425f26583a..6f32f44128e1 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -135,33 +135,37 @@  static int fill_cache_write(unsigned char *buf, size_t buf_size, bool once)
 	return 0;
 }
 
-static int fill_cache(size_t buf_size, int memflush, int op, bool once)
+static unsigned char *alloc_buffer(size_t buf_size, int memflush)
 {
 	unsigned char *buf;
-	int ret;
 
 	buf = malloc_and_init_memory(buf_size);
 	if (!buf)
-		return -1;
+		return NULL;
 
 	/* Flush the memory before using to avoid "cache hot pages" effect */
 	if (memflush)
 		mem_flush(buf, buf_size);
 
+	return buf;
+}
+
+static int fill_cache(size_t buf_size, int memflush, int op, bool once)
+{
+	unsigned char *buf;
+	int ret;
+
+	buf = alloc_buffer(buf_size, memflush);
+	if (!buf)
+		return -1;
+
 	if (op == 0)
 		ret = fill_cache_read(buf, buf_size, once);
 	else
 		ret = fill_cache_write(buf, buf_size, once);
-
 	free(buf);
 
-	if (ret) {
-		printf("\n Error in fill cache read/write...\n");
-		return -1;
-	}
-
-
-	return 0;
+	return ret;
 }
 
 int run_fill_buf(size_t span, int memflush, int op, bool once)