| Message ID | 20231024092634.7122-3-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: CAT test improvements & generalized test framework | expand |
On 2023-10-24 at 12:26:12 +0300, Ilpo Järvinen wrote: >There are unnecessary nested calls in fill_buf.c: > - run_fill_buf() calls fill_cache() > - alloc_buffer() calls malloc_and_init_memory() > >Simplify the code flow and remove those unnecessary call levels by >moving the called code inside the calling function. > >Resolve the difference in run_fill_buf() and fill_cache() parameter >name into 'buf_size' which is more descriptive than 'span'. > >Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >--- > tools/testing/selftests/resctrl/fill_buf.c | 58 +++++++--------------- > tools/testing/selftests/resctrl/resctrl.h | 2 +- > 2 files changed, 20 insertions(+), 40 deletions(-) > >diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c >index f9893edda869..9d0b0bf4b85a 100644 >--- a/tools/testing/selftests/resctrl/fill_buf.c >+++ b/tools/testing/selftests/resctrl/fill_buf.c >@@ -51,29 +51,6 @@ static void mem_flush(unsigned char *buf, size_t buf_size) > sb(); > } > >-static void *malloc_and_init_memory(size_t buf_size) >-{ >- void *p = NULL; >- uint64_t *p64; >- size_t s64; >- int ret; >- >- ret = posix_memalign(&p, PAGE_SIZE, buf_size); >- if (ret < 0) >- return NULL; >- >- p64 = (uint64_t *)p; >- s64 = buf_size / sizeof(uint64_t); >- >- while (s64 > 0) { >- *p64 = (uint64_t)rand(); >- p64 += (CL_SIZE / sizeof(uint64_t)); >- s64 -= (CL_SIZE / sizeof(uint64_t)); >- } >- >- return p; >-} >- > static int fill_one_span_read(unsigned char *buf, size_t buf_size) > { > unsigned char *end_ptr = buf + buf_size; >@@ -137,20 +114,33 @@ static int fill_cache_write(unsigned char *buf, size_t buf_size, bool once) > > static unsigned char *alloc_buffer(size_t buf_size, int memflush) > { >- unsigned char *buf; >+ void *p = NULL; Is this initialization doing anything? "p" seems to be either overwritten or in case of an error never accessed. >+ uint64_t *p64; >+ size_t s64; >+ int ret; > >- buf = malloc_and_init_memory(buf_size); >- if (!buf) >+ ret = posix_memalign(&p, PAGE_SIZE, buf_size); >+ if (ret < 0) > return NULL; > >+ /* Initialize the buffer */ >+ p64 = (uint64_t *)p; >+ s64 = buf_size / sizeof(uint64_t); >+ >+ while (s64 > 0) { >+ *p64 = (uint64_t)rand(); >+ p64 += (CL_SIZE / sizeof(uint64_t)); >+ s64 -= (CL_SIZE / sizeof(uint64_t)); >+ } >+ > /* Flush the memory before using to avoid "cache hot pages" effect */ > if (memflush) >- mem_flush(buf, buf_size); >+ mem_flush(p, buf_size); Wouldn't renaming "p" to "buf" keep this relationship with "buf_size" more explicit? Or is naming void pointers "buffers" not appropriate? > >- return buf; >+ return p; > }
On Fri, 27 Oct 2023, Maciej Wieczór-Retman wrote: > On 2023-10-24 at 12:26:12 +0300, Ilpo Järvinen wrote: > >There are unnecessary nested calls in fill_buf.c: > > - run_fill_buf() calls fill_cache() > > - alloc_buffer() calls malloc_and_init_memory() > > > >Simplify the code flow and remove those unnecessary call levels by > >moving the called code inside the calling function. > > > >Resolve the difference in run_fill_buf() and fill_cache() parameter > >name into 'buf_size' which is more descriptive than 'span'. > > > >Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > >--- > > tools/testing/selftests/resctrl/fill_buf.c | 58 +++++++--------------- > > tools/testing/selftests/resctrl/resctrl.h | 2 +- > > 2 files changed, 20 insertions(+), 40 deletions(-) > > > >diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > >index f9893edda869..9d0b0bf4b85a 100644 > >--- a/tools/testing/selftests/resctrl/fill_buf.c > >+++ b/tools/testing/selftests/resctrl/fill_buf.c > >@@ -51,29 +51,6 @@ static void mem_flush(unsigned char *buf, size_t buf_size) > > sb(); > > } > > > >-static void *malloc_and_init_memory(size_t buf_size) > >-{ > >- void *p = NULL; > >- uint64_t *p64; > >- size_t s64; > >- int ret; > >- > >- ret = posix_memalign(&p, PAGE_SIZE, buf_size); > >- if (ret < 0) > >- return NULL; > >- > >- p64 = (uint64_t *)p; > >- s64 = buf_size / sizeof(uint64_t); > >- > >- while (s64 > 0) { > >- *p64 = (uint64_t)rand(); > >- p64 += (CL_SIZE / sizeof(uint64_t)); > >- s64 -= (CL_SIZE / sizeof(uint64_t)); > >- } > >- > >- return p; > >-} > >- > > static int fill_one_span_read(unsigned char *buf, size_t buf_size) > > { > > unsigned char *end_ptr = buf + buf_size; > >@@ -137,20 +114,33 @@ static int fill_cache_write(unsigned char *buf, size_t buf_size, bool once) > > > > static unsigned char *alloc_buffer(size_t buf_size, int memflush) > > { > >- unsigned char *buf; > >+ void *p = NULL; > > Is this initialization doing anything? "p" seems to be either overwritten or in > case of an error never accessed. I'm aware of that but the compiler is too stupid to know that p is initialized if there's no error and spits out a warning so I'll have to keep the unnecessary initialization. > >+ uint64_t *p64; > >+ size_t s64; > >+ int ret; > > > >- buf = malloc_and_init_memory(buf_size); > >- if (!buf) > >+ ret = posix_memalign(&p, PAGE_SIZE, buf_size); > >+ if (ret < 0) > > return NULL; > > > >+ /* Initialize the buffer */ > >+ p64 = (uint64_t *)p; > >+ s64 = buf_size / sizeof(uint64_t); > >+ > >+ while (s64 > 0) { > >+ *p64 = (uint64_t)rand(); > >+ p64 += (CL_SIZE / sizeof(uint64_t)); > >+ s64 -= (CL_SIZE / sizeof(uint64_t)); > >+ } > >+ > > /* Flush the memory before using to avoid "cache hot pages" effect */ > > if (memflush) > >- mem_flush(buf, buf_size); > >+ mem_flush(p, buf_size); > > Wouldn't renaming "p" to "buf" keep this relationship with "buf_size" more > explicit? I'll change it to buf. This patch has a long history which preceeds the change where I made the buffer ptr naming more consistent and I didn't realize I departed here again from the consistent naming until you now pointed it out.
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index f9893edda869..9d0b0bf4b85a 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -51,29 +51,6 @@ static void mem_flush(unsigned char *buf, size_t buf_size) sb(); } -static void *malloc_and_init_memory(size_t buf_size) -{ - void *p = NULL; - uint64_t *p64; - size_t s64; - int ret; - - ret = posix_memalign(&p, PAGE_SIZE, buf_size); - if (ret < 0) - return NULL; - - p64 = (uint64_t *)p; - s64 = buf_size / sizeof(uint64_t); - - while (s64 > 0) { - *p64 = (uint64_t)rand(); - p64 += (CL_SIZE / sizeof(uint64_t)); - s64 -= (CL_SIZE / sizeof(uint64_t)); - } - - return p; -} - static int fill_one_span_read(unsigned char *buf, size_t buf_size) { unsigned char *end_ptr = buf + buf_size; @@ -137,20 +114,33 @@ static int fill_cache_write(unsigned char *buf, size_t buf_size, bool once) static unsigned char *alloc_buffer(size_t buf_size, int memflush) { - unsigned char *buf; + void *p = NULL; + uint64_t *p64; + size_t s64; + int ret; - buf = malloc_and_init_memory(buf_size); - if (!buf) + ret = posix_memalign(&p, PAGE_SIZE, buf_size); + if (ret < 0) return NULL; + /* Initialize the buffer */ + p64 = (uint64_t *)p; + s64 = buf_size / sizeof(uint64_t); + + while (s64 > 0) { + *p64 = (uint64_t)rand(); + p64 += (CL_SIZE / sizeof(uint64_t)); + s64 -= (CL_SIZE / sizeof(uint64_t)); + } + /* Flush the memory before using to avoid "cache hot pages" effect */ if (memflush) - mem_flush(buf, buf_size); + mem_flush(p, buf_size); - return buf; + return p; } -static int fill_cache(size_t buf_size, int memflush, int op, bool once) +int run_fill_buf(size_t buf_size, int memflush, int op, bool once) { unsigned char *buf; int ret; @@ -164,16 +154,6 @@ static int fill_cache(size_t buf_size, int memflush, int op, bool once) else ret = fill_cache_write(buf, buf_size, once); free(buf); - - return ret; -} - -int run_fill_buf(size_t span, int memflush, int op, bool once) -{ - size_t cache_size = span; - int ret; - - ret = fill_cache(cache_size, memflush, op, once); if (ret) { printf("\n Error in fill cache\n"); return -1; diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index a33f414f6019..08b95b5a4949 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -92,7 +92,7 @@ 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); -int run_fill_buf(size_t span, int memflush, int op, bool once); +int run_fill_buf(size_t buf_size, int memflush, int op, bool once); int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param); int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd); void tests_cleanup(void);
There are unnecessary nested calls in fill_buf.c: - run_fill_buf() calls fill_cache() - alloc_buffer() calls malloc_and_init_memory() Simplify the code flow and remove those unnecessary call levels by moving the called code inside the calling function. Resolve the difference in run_fill_buf() and fill_cache() parameter name into 'buf_size' which is more descriptive than 'span'. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- tools/testing/selftests/resctrl/fill_buf.c | 58 +++++++--------------- tools/testing/selftests/resctrl/resctrl.h | 2 +- 2 files changed, 20 insertions(+), 40 deletions(-)