| Message ID | 20230418114506.46788-11-ilpo.jarvinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Fixes, cleanups, and rewritten CAT test | expand |
Hi Ilpo, On 4/18/2023 4:44 AM, Ilpo Järvinen wrote: > MBM, MBA and CMT test cases use run_fill_buf() to loop indefinitely > loop around the buffer. CAT test case is different and doesn't want to "loop indefinitely loop" -> "loop indefinitely" ? > loop around the buffer continuously. > > Split run_fill_buf() into helpers so that both the use cases are easier > to control by creating separate functions for buffer allocation, > looping around the buffer and the deallocation. Make those functions > available for tests. > > 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 | 46 ++++++++++++++++------ > tools/testing/selftests/resctrl/resctrl.h | 3 ++ > 2 files changed, 37 insertions(+), 12 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > index 5cdb421a2f6c..6f0438aa71a6 100644 > --- a/tools/testing/selftests/resctrl/fill_buf.c > +++ b/tools/testing/selftests/resctrl/fill_buf.c > @@ -24,6 +24,11 @@ > > static unsigned char *startptr; > > +void free_buffer(void) > +{ > + free(startptr); > +} > +
On Fri, 21 Apr 2023, Reinette Chatre wrote: > On 4/18/2023 4:44 AM, Ilpo Järvinen wrote: > > > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > > index 5cdb421a2f6c..6f0438aa71a6 100644 > > --- a/tools/testing/selftests/resctrl/fill_buf.c > > +++ b/tools/testing/selftests/resctrl/fill_buf.c > > @@ -24,6 +24,11 @@ > > > > static unsigned char *startptr; > > > > +void free_buffer(void) > > +{ > > + free(startptr); > > +} > > + > > >From what I understand startptr is a global variable because there used > to be a signal handler that attempted to free the buffer as part of > its cleanup. This was not necessary and this behavior no longer exists, > yet the global buffer pointer remains. > See commit 73c55fa5ab55 ("selftests/resctrl: Commonize the signal handler > register/unregister for all tests") > > I do not see why a global buffer pointer with all these indirections > are needed. Why not just use a local pointer and pass it to functions > as needed? In the above case, just call free(pointer) directly from the > test. OK, I'll try to convert all this into using non-global pointers then. It requires a bit refactoring but, IIRC, it is doable. > > static void sb(void) > > { > > #if defined(__i386) || defined(__x86_64) > > @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, > > return 0; > > } > > > > -static int > > -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val) > > +int alloc_buffer(unsigned long long buf_size, int memflush) > > { > > This can be an allocation function that returns a pointer to > allocated buffer, NULL if error. > > > - unsigned char *start_ptr, *end_ptr; > > - int ret; > > + unsigned char *start_ptr; > > > > start_ptr = malloc_and_init_memory(buf_size); > > if (!start_ptr) > > return -1; > > > > startptr = start_ptr; > > - end_ptr = start_ptr + buf_size; > > > > /* Flush the memory before using to avoid "cache hot pages" effect */ > > if (memflush) > > mem_flush(start_ptr, buf_size); > > > > + return 0; > > +} > > + > > +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val) > > +{ > > + unsigned char *end_ptr; > > + int ret; > > + > > + end_ptr = startptr + buf_size; > > if (op == 0) > > - ret = fill_cache_read(start_ptr, end_ptr, resctrl_val); > > + ret = fill_cache_read(startptr, end_ptr, resctrl_val); > > else > > - ret = fill_cache_write(start_ptr, end_ptr, resctrl_val); > > + ret = fill_cache_write(startptr, end_ptr, resctrl_val); > > > > - if (ret) { > > + if (ret) > > printf("\n Error in fill cache read/write...\n"); > > - return -1; > > - } > > > > - free(startptr); > > + return ret; > > +} > > > > This seems like an unnecessary level of abstraction to me. Could > callers not just call fill_cache_read()/fill_cache_write() directly? > I think doing so will make tests easier to understand. Looking ahead > at how cat_val() turns out in the final patch I do think a call > to fill_cache_read() is easier to follow than this abstraction. Passing a custom benchmark command with -b would lose some functionality if this abstraction is removed. CAT test could make a direct call though as it doesn't care about the benchmark command. How useful that -b functionality is for selftesting is somewhat questionable though.
Hi Ilpo, On 4/24/2023 9:01 AM, Ilpo Järvinen wrote: > On Fri, 21 Apr 2023, Reinette Chatre wrote: >> On 4/18/2023 4:44 AM, Ilpo Järvinen wrote: >>> ... >>> static void sb(void) >>> { >>> #if defined(__i386) || defined(__x86_64) >>> @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, >>> return 0; >>> } >>> >>> -static int >>> -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val) >>> +int alloc_buffer(unsigned long long buf_size, int memflush) >>> { >> >> This can be an allocation function that returns a pointer to >> allocated buffer, NULL if error. >> >>> - unsigned char *start_ptr, *end_ptr; >>> - int ret; >>> + unsigned char *start_ptr; >>> >>> start_ptr = malloc_and_init_memory(buf_size); >>> if (!start_ptr) >>> return -1; >>> >>> startptr = start_ptr; >>> - end_ptr = start_ptr + buf_size; >>> >>> /* Flush the memory before using to avoid "cache hot pages" effect */ >>> if (memflush) >>> mem_flush(start_ptr, buf_size); >>> >>> + return 0; >>> +} >>> + >>> +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val) >>> +{ >>> + unsigned char *end_ptr; >>> + int ret; >>> + >>> + end_ptr = startptr + buf_size; >>> if (op == 0) >>> - ret = fill_cache_read(start_ptr, end_ptr, resctrl_val); >>> + ret = fill_cache_read(startptr, end_ptr, resctrl_val); >>> else >>> - ret = fill_cache_write(start_ptr, end_ptr, resctrl_val); >>> + ret = fill_cache_write(startptr, end_ptr, resctrl_val); >>> >>> - if (ret) { >>> + if (ret) >>> printf("\n Error in fill cache read/write...\n"); >>> - return -1; >>> - } >>> >>> - free(startptr); >>> + return ret; >>> +} >>> >> >> This seems like an unnecessary level of abstraction to me. Could >> callers not just call fill_cache_read()/fill_cache_write() directly? >> I think doing so will make tests easier to understand. Looking ahead >> at how cat_val() turns out in the final patch I do think a call >> to fill_cache_read() is easier to follow than this abstraction. > > Passing a custom benchmark command with -b would lose some functionality > if this abstraction is removed. CAT test could make a direct call though > as it doesn't care about the benchmark command. > > How useful that -b functionality is for selftesting is somewhat > questionable though. I do not think we are speaking about the same thing here. I think that use_buffer() is unnecessary. fill_cache() can just call fill_cache_read() or fill_cache_write() directly, depending on the op value. Could you please elaborate how that impacts the custom benchmark? Looking ahead at patch 24/24: "selftests/resctrl: Rewrite Cache Allocation Technology (CAT) test" I feel more strongly that use_buffer() is unnecessary since it adds an unnecessary layer and makes it harder to see what the test does. Reinette
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 5cdb421a2f6c..6f0438aa71a6 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -24,6 +24,11 @@ static unsigned char *startptr; +void free_buffer(void) +{ + free(startptr); +} + static void sb(void) { #if defined(__i386) || defined(__x86_64) @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, return 0; } -static int -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val) +int alloc_buffer(unsigned long long buf_size, int memflush) { - unsigned char *start_ptr, *end_ptr; - int ret; + unsigned char *start_ptr; start_ptr = malloc_and_init_memory(buf_size); if (!start_ptr) return -1; startptr = start_ptr; - end_ptr = start_ptr + buf_size; /* Flush the memory before using to avoid "cache hot pages" effect */ if (memflush) mem_flush(start_ptr, buf_size); + return 0; +} + +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val) +{ + unsigned char *end_ptr; + int ret; + + end_ptr = startptr + buf_size; if (op == 0) - ret = fill_cache_read(start_ptr, end_ptr, resctrl_val); + ret = fill_cache_read(startptr, end_ptr, resctrl_val); else - ret = fill_cache_write(start_ptr, end_ptr, resctrl_val); + ret = fill_cache_write(startptr, end_ptr, resctrl_val); - if (ret) { + if (ret) printf("\n Error in fill cache read/write...\n"); - return -1; - } - free(startptr); + return ret; +} - return 0; +static int +fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val) +{ + int ret; + + ret = alloc_buffer(buf_size, memflush); + if (ret) + return ret; + + ret = use_buffer(buf_size, op, resctrl_val); + free_buffer(); + + return ret; } int run_fill_buf(unsigned long span, int memflush, int op, char *resctrl_val) diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 574adac8932d..75bfa2b2746d 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -95,6 +95,9 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, char *resctrl_val); int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu, int group_fd, unsigned long flags); +void free_buffer(void); +int alloc_buffer(unsigned long long buf_size, int memflush); +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val); int run_fill_buf(unsigned long span, int memflush, int op, char *resctrl_val); int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param); int mbm_bw_change(unsigned long span, int cpu_no, char *bw_report, char **benchmark_cmd);