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);
elseret = fill_cache_read(startptr, end_ptr, resctrl_val);
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.