| Message ID | 43b368952bb006ee973311d9c9ae0eb53d8e7f60.1583657204.git.sai.praneeth.prakhya@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Miscellaneous fixes for resctrl selftests | expand |
Hi Sai, On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > Currently fill_buf (in-built benchmark) runs as a separate process and it > runs indefinitely looping around given buffer either reading it or writing > to it. But, some future test cases might want to start and stop looping > around the buffer as they see fit. So, modularize fill_buf to support this > use case. > > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> > --- > tools/testing/selftests/resctrl/fill_buf.c | 66 ++++++++++++++++++++---------- > 1 file changed, 44 insertions(+), 22 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > index 9ede7b63f059..204ae8870a32 100644 > --- a/tools/testing/selftests/resctrl/fill_buf.c > +++ b/tools/testing/selftests/resctrl/fill_buf.c > @@ -23,7 +23,7 @@ > #define PAGE_SIZE (4 * 1024) > #define MB (1024 * 1024) > > -static unsigned char *startptr; > +static unsigned char *startptr, *endptr; > > static void sb(void) > { > @@ -82,13 +82,13 @@ static void *malloc_and_init_memory(size_t s) > return p; > } > > -static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr) > +static int fill_one_span_read(void) > { > unsigned char sum, *p; > > sum = 0; > - p = start_ptr; > - while (p < end_ptr) { > + p = startptr; > + while (p < endptr) { > sum += *p; > p += (CL_SIZE / 2); > } > @@ -96,26 +96,24 @@ static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr) > return sum; > } > > -static > -void fill_one_span_write(unsigned char *start_ptr, unsigned char *end_ptr) > +static void fill_one_span_write(void) > { > unsigned char *p; > > - p = start_ptr; > - while (p < end_ptr) { > + p = startptr; > + while (p < endptr) { > *p = '1'; > p += (CL_SIZE / 2); > } > } > > -static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr, > - char *resctrl_val) > +static int fill_cache_read(char *resctrl_val) > { > int ret = 0; > FILE *fp; > > while (1) { > - ret = fill_one_span_read(start_ptr, end_ptr); > + ret = fill_one_span_read(); > if (!strcmp(resctrl_val, "cat")) > break; > } > @@ -130,11 +128,10 @@ static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr, > return 0; > } > > -static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, > - char *resctrl_val) > +static int fill_cache_write(char *resctrl_val) > { > while (1) { > - fill_one_span_write(start_ptr, end_ptr); > + fill_one_span_write(); > if (!strcmp(resctrl_val, "cat")) > break; > } > @@ -142,24 +139,25 @@ 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 malloc_and_init, int memflush, > - int op, char *resctrl_val) > +static > +int init_buffer(unsigned long long buf_size, int malloc_and_init, int memflush) > { > unsigned char *start_ptr, *end_ptr; > unsigned long long i; > - int ret; > > if (malloc_and_init) > start_ptr = malloc_and_init_memory(buf_size); > else > start_ptr = malloc(buf_size); > > - if (!start_ptr) > + if (!start_ptr) { > + printf("Failed to allocate memory to buffer\n"); > return -1; > + } > > - startptr = start_ptr; > end_ptr = start_ptr + buf_size; > + endptr = end_ptr; > + startptr = start_ptr; > > /* > * It's better to touch the memory once to avoid any compiler > @@ -176,16 +174,40 @@ fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush, > if (memflush) > mem_flush(start_ptr, buf_size); > > + return 0; > +} > + > +static int use_buffer_forever(int op, char *resctrl_val) > +{ > + int ret; > + > if (op == 0) > - ret = fill_cache_read(start_ptr, end_ptr, resctrl_val); > + ret = fill_cache_read(resctrl_val); > else > - ret = fill_cache_write(start_ptr, end_ptr, resctrl_val); > + ret = fill_cache_write(resctrl_val); > > if (ret) { > printf("\n Errror in fill cache read/write...\n"); > return -1; > } > > + return 0; > +} > + > +static int > +fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush, > + int op, char *resctrl_val) > +{ > + int ret; > + > + ret = init_buffer(buf_size, malloc_and_init, memflush); > + if (ret) > + return ret; > + > + ret = use_buffer_forever(op, resctrl_val); > + if (ret) > + return ret; Should buffer be freed on this error path? I think the asymmetrical nature of the memory allocation and release creates traps like this. It may be less error prone to have the pointer returned by init_buffer and the acted on and released within fill_cache(), passed to "use_buffer_forever()" as a parameter. The buffer size is known here, there is no need to keep an "end pointer" around. > + > free(startptr); > > return 0; > Reinette
Hi Reinette, On Tue, 2020-03-10 at 14:59 -0700, Reinette Chatre wrote: > Hi Sai, > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > > Currently fill_buf (in-built benchmark) runs as a separate process and it > > runs indefinitely looping around given buffer either reading it or writing > > to it. But, some future test cases might want to start and stop looping > > around the buffer as they see fit. So, modularize fill_buf to support this > > use case. > > > > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> > > --- > > tools/testing/selftests/resctrl/fill_buf.c | 66 ++++++++++++++++++++----- > > ----- > > 1 file changed, 44 insertions(+), 22 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c > > b/tools/testing/selftests/resctrl/fill_buf.c > > index 9ede7b63f059..204ae8870a32 100644 > > --- a/tools/testing/selftests/resctrl/fill_buf.c > > +++ b/tools/testing/selftests/resctrl/fill_buf.c > > @@ -23,7 +23,7 @@ > > #define PAGE_SIZE (4 * 1024) > > #define MB (1024 * 1024) > > > > -static unsigned char *startptr; > > +static unsigned char *startptr, *endptr; [Snipped.. assuming code over here might not be needed for discussion] > > +static int use_buffer_forever(int op, char *resctrl_val) > > +{ > > + int ret; > > + > > if (op == 0) > > - ret = fill_cache_read(start_ptr, end_ptr, resctrl_val); > > + ret = fill_cache_read(resctrl_val); > > else > > - ret = fill_cache_write(start_ptr, end_ptr, resctrl_val); > > + ret = fill_cache_write(resctrl_val); > > > > if (ret) { > > printf("\n Errror in fill cache read/write...\n"); > > return -1; > > } > > > > + return 0; > > +} > > + > > +static int > > +fill_cache(unsigned long long buf_size, int malloc_and_init, int > > memflush, > > + int op, char *resctrl_val) > > +{ > > + int ret; > > + > > + ret = init_buffer(buf_size, malloc_and_init, memflush); > > + if (ret) > > + return ret; > > + > > + ret = use_buffer_forever(op, resctrl_val); > > + if (ret) > > + return ret; > > Should buffer be freed on this error path? Yes, that's right.. my bad. Will fix it. But the right fix is, use_buffer_forever() should not return at all. It's meant to loop around the buffer _forever_. > I think the asymmetrical nature of the memory allocation and release > creates traps like this. > > It may be less error prone to have the pointer returned by init_buffer > and the acted on and released within fill_cache(), passed to > "use_buffer_forever()" as a parameter. The buffer size is known here, > there is no need to keep an "end pointer" around. The main reason for having "startptr" as a global variable is to free memory when fill_buf is killed. fill_buf runs as a separate process (for test cases like MBM, MBA and CQM) and when user issues Ctrl_c or when the test kills benchmark_pid (i.e. fill_buf), the buffer is freed (please see ctrl_handler()). So, I thought, as "startptr" is anyways global, why pass it around as an argument? While making this change I thought it's natural to make "endptr" global as well because the function didn't really look good to just take endptr as an argument without startptr. I do agree that asymmetrical nature of the memory allocation and release might create traps, I will try to overcome this for CAT test case (other test cases will not need it). Regards, Sai
Hi Sai, On 3/10/2020 6:04 PM, Sai Praneeth Prakhya wrote: > On Tue, 2020-03-10 at 14:59 -0700, Reinette Chatre wrote: >> On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: >>> Currently fill_buf (in-built benchmark) runs as a separate process and it >>> runs indefinitely looping around given buffer either reading it or writing >>> to it. But, some future test cases might want to start and stop looping >>> around the buffer as they see fit. So, modularize fill_buf to support this >>> use case. >>> >>> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> >>> --- >>> tools/testing/selftests/resctrl/fill_buf.c | 66 ++++++++++++++++++++----- >>> ----- >>> 1 file changed, 44 insertions(+), 22 deletions(-) >>> >>> diff --git a/tools/testing/selftests/resctrl/fill_buf.c >>> b/tools/testing/selftests/resctrl/fill_buf.c >>> index 9ede7b63f059..204ae8870a32 100644 >>> --- a/tools/testing/selftests/resctrl/fill_buf.c >>> +++ b/tools/testing/selftests/resctrl/fill_buf.c >>> @@ -23,7 +23,7 @@ >>> #define PAGE_SIZE (4 * 1024) >>> #define MB (1024 * 1024) >>> >>> -static unsigned char *startptr; >>> +static unsigned char *startptr, *endptr; > > [Snipped.. assuming code over here might not be needed for discussion] > >>> +static int use_buffer_forever(int op, char *resctrl_val) >>> +{ >>> + int ret; >>> + >>> if (op == 0) >>> - ret = fill_cache_read(start_ptr, end_ptr, resctrl_val); >>> + ret = fill_cache_read(resctrl_val); >>> else >>> - ret = fill_cache_write(start_ptr, end_ptr, resctrl_val); >>> + ret = fill_cache_write(resctrl_val); >>> >>> if (ret) { >>> printf("\n Errror in fill cache read/write...\n"); >>> return -1; >>> } >>> >>> + return 0; >>> +} >>> + >>> +static int >>> +fill_cache(unsigned long long buf_size, int malloc_and_init, int >>> memflush, >>> + int op, char *resctrl_val) >>> +{ >>> + int ret; >>> + >>> + ret = init_buffer(buf_size, malloc_and_init, memflush); >>> + if (ret) >>> + return ret; >>> + >>> + ret = use_buffer_forever(op, resctrl_val); >>> + if (ret) >>> + return ret; >> >> Should buffer be freed on this error path? > > Yes, that's right.. my bad. Will fix it. But the right fix is, > use_buffer_forever() should not return at all. It's meant to loop around the > buffer _forever_. > >> I think the asymmetrical nature of the memory allocation and release >> creates traps like this. >> >> It may be less error prone to have the pointer returned by init_buffer >> and the acted on and released within fill_cache(), passed to >> "use_buffer_forever()" as a parameter. The buffer size is known here, >> there is no need to keep an "end pointer" around. > > The main reason for having "startptr" as a global variable is to free memory > when fill_buf is killed. fill_buf runs as a separate process (for test cases > like MBM, MBA and CQM) and when user issues Ctrl_c or when the test kills > benchmark_pid (i.e. fill_buf), the buffer is freed (please see > ctrl_handler()). I see. Got it, thanks. > > So, I thought, as "startptr" is anyways global, why pass it around as an > argument? While making this change I thought it's natural to make "endptr" > global as well because the function didn't really look good to just take > endptr as an argument without startptr. Maintaining the end pointer is unusual. The start of the buffer and the size are known properties that the end of the buffer can be computed from. Not a problem, it just seems inconsistent that some of the buffer functions operate on the start pointer and size while others operate on the start pointer and end pointer. > I do agree that asymmetrical nature of the memory allocation and release might > create traps, I will try to overcome this for CAT test case (other test cases > will not need it). Thank you very much Reinette
Hi Reinette, On Wed, 2020-03-11 at 08:44 -0700, Reinette Chatre wrote: > Hi Sai, > > On 3/10/2020 6:04 PM, Sai Praneeth Prakhya wrote: > > On Tue, 2020-03-10 at 14:59 -0700, Reinette Chatre wrote: > > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > > > > Currently fill_buf (in-built benchmark) runs as a separate process a > > > > > > > [SNIP] > > > Should buffer be freed on this error path? > > > > Yes, that's right.. my bad. Will fix it. But the right fix is, > > use_buffer_forever() should not return at all. It's meant to loop around > > the > > buffer _forever_. > > > > > I think the asymmetrical nature of the memory allocation and release > > > creates traps like this. > > > > > > It may be less error prone to have the pointer returned by init_buffer > > > and the acted on and released within fill_cache(), passed to > > > "use_buffer_forever()" as a parameter. The buffer size is known here, > > > there is no need to keep an "end pointer" around. > > > > The main reason for having "startptr" as a global variable is to free > > memory > > when fill_buf is killed. fill_buf runs as a separate process (for test > > cases > > like MBM, MBA and CQM) and when user issues Ctrl_c or when the test kills > > benchmark_pid (i.e. fill_buf), the buffer is freed (please see > > ctrl_handler()). > > I see. Got it, thanks. > > > So, I thought, as "startptr" is anyways global, why pass it around as an > > argument? While making this change I thought it's natural to make "endptr" > > global as well because the function didn't really look good to just take > > endptr as an argument without startptr. > > Maintaining the end pointer is unusual. The start of the buffer and the > size are known properties that the end of the buffer can be computed > from. Not a problem, it just seems inconsistent that some of the buffer > functions operate on the start pointer and size while others operate on > the start pointer and end pointer. Ok.. makes sense. I will try to make it consistent by using endptr all the time. One advantage of using endptr is that we could just compute endptr once and use it when needed by passing it as variable (will try to not make it global variable). Regards, Sai
Hi Sai, On 3/11/2020 10:45 AM, Sai Praneeth Prakhya wrote: > On Wed, 2020-03-11 at 08:44 -0700, Reinette Chatre wrote: >> On 3/10/2020 6:04 PM, Sai Praneeth Prakhya wrote: >>> On Tue, 2020-03-10 at 14:59 -0700, Reinette Chatre wrote: >>>> On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: >>>>> Currently fill_buf (in-built benchmark) runs as a separate process a >>>>> >>>> > > [SNIP] > >>>> Should buffer be freed on this error path? >>> >>> Yes, that's right.. my bad. Will fix it. But the right fix is, >>> use_buffer_forever() should not return at all. It's meant to loop around >>> the >>> buffer _forever_. >>> >>>> I think the asymmetrical nature of the memory allocation and release >>>> creates traps like this. >>>> >>>> It may be less error prone to have the pointer returned by init_buffer >>>> and the acted on and released within fill_cache(), passed to >>>> "use_buffer_forever()" as a parameter. The buffer size is known here, >>>> there is no need to keep an "end pointer" around. >>> >>> The main reason for having "startptr" as a global variable is to free >>> memory >>> when fill_buf is killed. fill_buf runs as a separate process (for test >>> cases >>> like MBM, MBA and CQM) and when user issues Ctrl_c or when the test kills >>> benchmark_pid (i.e. fill_buf), the buffer is freed (please see >>> ctrl_handler()). >> >> I see. Got it, thanks. >> >>> So, I thought, as "startptr" is anyways global, why pass it around as an >>> argument? While making this change I thought it's natural to make "endptr" >>> global as well because the function didn't really look good to just take >>> endptr as an argument without startptr. >> >> Maintaining the end pointer is unusual. The start of the buffer and the >> size are known properties that the end of the buffer can be computed >> from. Not a problem, it just seems inconsistent that some of the buffer >> functions operate on the start pointer and size while others operate on >> the start pointer and end pointer. > > Ok.. makes sense. I will try to make it consistent by using endptr all the > time. One advantage of using endptr is that we could just compute endptr once > and use it when needed by passing it as variable (will try to not make it > global variable). This may add unnecessary complexity because from what I can tell some of those calls require buffer size and this would then require needing to recompute the buffer size based on the start and end pointers. Do you really need the end pointer? Can you not just use the start pointer and buffer size? Reinette
HI Reinette, On Wed, 2020-03-11 at 11:10 -0700, Reinette Chatre wrote: > Hi Sai, > > On 3/11/2020 10:45 AM, Sai Praneeth Prakhya wrote: > > On Wed, 2020-03-11 at 08:44 -0700, Reinette Chatre wrote: > > > On 3/10/2020 6:04 PM, Sai Praneeth Prakhya wrote: > > > > On Tue, 2020-03-10 at 14:59 -0700, Reinette Chatre wrote: > > > > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > > > > > > Currently fill_buf (in-built benchmark) runs as a separate process > > > > > > a > > > > > > > > [SNIP] > > > Maintaining the end pointer is unusual. The start of the buffer and the > > > size are known properties that the end of the buffer can be computed > > > from. Not a problem, it just seems inconsistent that some of the buffer > > > functions operate on the start pointer and size while others operate on > > > the start pointer and end pointer. > > > > Ok.. makes sense. I will try to make it consistent by using endptr all the > > time. One advantage of using endptr is that we could just compute endptr > > once > > and use it when needed by passing it as variable (will try to not make it > > global variable). > > This may add unnecessary complexity because from what I can tell some of > those calls require buffer size and this would then require needing to > recompute the buffer size based on the start and end pointers. Do you > really need the end pointer? Can you not just use the start pointer and > buffer size? Ok.. makes sense. Will use buffer size. Regards, Sai
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 9ede7b63f059..204ae8870a32 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -23,7 +23,7 @@ #define PAGE_SIZE (4 * 1024) #define MB (1024 * 1024) -static unsigned char *startptr; +static unsigned char *startptr, *endptr; static void sb(void) { @@ -82,13 +82,13 @@ static void *malloc_and_init_memory(size_t s) return p; } -static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr) +static int fill_one_span_read(void) { unsigned char sum, *p; sum = 0; - p = start_ptr; - while (p < end_ptr) { + p = startptr; + while (p < endptr) { sum += *p; p += (CL_SIZE / 2); } @@ -96,26 +96,24 @@ static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr) return sum; } -static -void fill_one_span_write(unsigned char *start_ptr, unsigned char *end_ptr) +static void fill_one_span_write(void) { unsigned char *p; - p = start_ptr; - while (p < end_ptr) { + p = startptr; + while (p < endptr) { *p = '1'; p += (CL_SIZE / 2); } } -static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr, - char *resctrl_val) +static int fill_cache_read(char *resctrl_val) { int ret = 0; FILE *fp; while (1) { - ret = fill_one_span_read(start_ptr, end_ptr); + ret = fill_one_span_read(); if (!strcmp(resctrl_val, "cat")) break; } @@ -130,11 +128,10 @@ static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr, return 0; } -static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, - char *resctrl_val) +static int fill_cache_write(char *resctrl_val) { while (1) { - fill_one_span_write(start_ptr, end_ptr); + fill_one_span_write(); if (!strcmp(resctrl_val, "cat")) break; } @@ -142,24 +139,25 @@ 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 malloc_and_init, int memflush, - int op, char *resctrl_val) +static +int init_buffer(unsigned long long buf_size, int malloc_and_init, int memflush) { unsigned char *start_ptr, *end_ptr; unsigned long long i; - int ret; if (malloc_and_init) start_ptr = malloc_and_init_memory(buf_size); else start_ptr = malloc(buf_size); - if (!start_ptr) + if (!start_ptr) { + printf("Failed to allocate memory to buffer\n"); return -1; + } - startptr = start_ptr; end_ptr = start_ptr + buf_size; + endptr = end_ptr; + startptr = start_ptr; /* * It's better to touch the memory once to avoid any compiler @@ -176,16 +174,40 @@ fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush, if (memflush) mem_flush(start_ptr, buf_size); + return 0; +} + +static int use_buffer_forever(int op, char *resctrl_val) +{ + int ret; + if (op == 0) - ret = fill_cache_read(start_ptr, end_ptr, resctrl_val); + ret = fill_cache_read(resctrl_val); else - ret = fill_cache_write(start_ptr, end_ptr, resctrl_val); + ret = fill_cache_write(resctrl_val); if (ret) { printf("\n Errror in fill cache read/write...\n"); return -1; } + return 0; +} + +static int +fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush, + int op, char *resctrl_val) +{ + int ret; + + ret = init_buffer(buf_size, malloc_and_init, memflush); + if (ret) + return ret; + + ret = use_buffer_forever(op, resctrl_val); + if (ret) + return ret; + free(startptr); return 0;
Currently fill_buf (in-built benchmark) runs as a separate process and it runs indefinitely looping around given buffer either reading it or writing to it. But, some future test cases might want to start and stop looping around the buffer as they see fit. So, modularize fill_buf to support this use case. Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> --- tools/testing/selftests/resctrl/fill_buf.c | 66 ++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 22 deletions(-)