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);
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