Re: [PATCH V2 08/13] selftests/resctrl: Remove unused measurement code

-- i. > --- > Changes since V1: > - New patch. > --- > tools/testing/selftests/resctrl/mba_test.c | 4 +- > tools/testing/selftests/resctrl/mbm_test.c | 4 +- > tools/testing/selftests/resctrl/resctrl.h | 8 +- > tools/testing/selftests/resctrl/resctrl_val.c | 234 ++++++------------ > tools/testing/selftests/resctrl/resctrlfs.c | 17 -- > 5 files changed, 85 insertions(+), 182 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c > index da40a8ed4413..be0ead73e55d 100644 > --- a/tools/testing/selftests/resctrl/mba_test.c > +++ b/tools/testing/selftests/resctrl/mba_test.c > @@ -21,7 +21,7 @@ static int mba_init(const struct resctrl_val_param *param, int domain_id) > { > int ret; > > - ret = initialize_mem_bw_imc(); > + ret = initialize_read_mem_bw_imc(); > if (ret) > return ret; > > @@ -68,7 +68,7 @@ static int mba_setup(const struct resctrl_test *test, > static int mba_measure(const struct user_params *uparams, > struct resctrl_val_param *param, pid_t bm_pid) > { > - return measure_mem_bw(uparams, param, bm_pid, "reads"); > + return measure_read_mem_bw(uparams, param, bm_pid); > } > > static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) > diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c > index 80c7a1bc13b8..9744bf1ac16f 100644 > --- a/tools/testing/selftests/resctrl/mbm_test.c > +++ b/tools/testing/selftests/resctrl/mbm_test.c > @@ -91,7 +91,7 @@ static int mbm_init(const struct resctrl_val_param *param, int domain_id) > { > int ret; > > - ret = initialize_mem_bw_imc(); > + ret = initialize_read_mem_bw_imc(); > if (ret) > return ret; > > @@ -122,7 +122,7 @@ static int mbm_setup(const struct resctrl_test *test, > static int mbm_measure(const struct user_params *uparams, > struct resctrl_val_param *param, pid_t bm_pid) > { > - return measure_mem_bw(uparams, param, bm_pid, "reads"); > + return measure_read_mem_bw(uparams, param, bm_pid); > } > > static void mbm_test_cleanup(void) > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index ba1ce1b35699..82801245e4c1 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -126,7 +126,6 @@ int filter_dmesg(void); > int get_domain_id(const char *resource, int cpu_no, int *domain_id); > int mount_resctrlfs(void); > int umount_resctrlfs(void); > -const char *get_bw_report_type(const char *bw_report); > bool resctrl_resource_exists(const char *resource); > bool resctrl_mon_feature_exists(const char *resource, const char *feature); > bool resource_info_file_exists(const char *resource, const char *file); > @@ -143,10 +142,9 @@ unsigned char *alloc_buffer(size_t buf_size, int memflush); > void mem_flush(unsigned char *buf, size_t buf_size); > void fill_cache_read(unsigned char *buf, size_t buf_size, bool once); > int run_fill_buf(size_t buf_size, int memflush); > -int initialize_mem_bw_imc(void); > -int measure_mem_bw(const struct user_params *uparams, > - struct resctrl_val_param *param, pid_t bm_pid, > - const char *bw_report); > +int initialize_read_mem_bw_imc(void); > +int measure_read_mem_bw(const struct user_params *uparams, > + struct resctrl_val_param *param, pid_t bm_pid); > void initialize_mem_bw_resctrl(const struct resctrl_val_param *param, > int domain_id); > int resctrl_val(const struct resctrl_test *test, > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 8b5973c5e934..a1e097dc8e05 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -12,13 +12,10 @@ > > #define UNCORE_IMC "uncore_imc" > #define READ_FILE_NAME "events/cas_count_read" > -#define WRITE_FILE_NAME "events/cas_count_write" > #define DYN_PMU_PATH "/sys/bus/event_source/devices" > #define SCALE 0.00006103515625 > #define MAX_IMCS 20 > #define MAX_TOKENS 5 > -#define READ 0 > -#define WRITE 1 > > #define CON_MBM_LOCAL_BYTES_PATH \ > "%s/%s/mon_data/mon_L3_%02d/mbm_local_bytes" > @@ -41,44 +38,43 @@ struct imc_counter_config { > > static char mbm_total_path[1024]; > static int imcs; > -static struct imc_counter_config imc_counters_config[MAX_IMCS][2]; > +static struct imc_counter_config imc_counters_config[MAX_IMCS]; > static const struct resctrl_test *current_test; > > -static void membw_initialize_perf_event_attr(int i, int j) > +static void read_mem_bw_initialize_perf_event_attr(int i) > { > - memset(&imc_counters_config[i][j].pe, 0, > + memset(&imc_counters_config[i].pe, 0, > sizeof(struct perf_event_attr)); > - imc_counters_config[i][j].pe.type = imc_counters_config[i][j].type; > - imc_counters_config[i][j].pe.size = sizeof(struct perf_event_attr); > - imc_counters_config[i][j].pe.disabled = 1; > - imc_counters_config[i][j].pe.inherit = 1; > - imc_counters_config[i][j].pe.exclude_guest = 0; > - imc_counters_config[i][j].pe.config = > - imc_counters_config[i][j].umask << 8 | > - imc_counters_config[i][j].event; > - imc_counters_config[i][j].pe.sample_type = PERF_SAMPLE_IDENTIFIER; > - imc_counters_config[i][j].pe.read_format = > + imc_counters_config[i].pe.type = imc_counters_config[i].type; > + imc_counters_config[i].pe.size = sizeof(struct perf_event_attr); > + imc_counters_config[i].pe.disabled = 1; > + imc_counters_config[i].pe.inherit = 1; > + imc_counters_config[i].pe.exclude_guest = 0; > + imc_counters_config[i].pe.config = > + imc_counters_config[i].umask << 8 | > + imc_counters_config[i].event; > + imc_counters_config[i].pe.sample_type = PERF_SAMPLE_IDENTIFIER; > + imc_counters_config[i].pe.read_format = > PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING; > } > > -static void membw_ioctl_perf_event_ioc_reset_enable(int i, int j) > +static void read_mem_bw_ioctl_perf_event_ioc_reset_enable(int i) > { > - ioctl(imc_counters_config[i][j].fd, PERF_EVENT_IOC_RESET, 0); > - ioctl(imc_counters_config[i][j].fd, PERF_EVENT_IOC_ENABLE, 0); > + ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_RESET, 0); > + ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_ENABLE, 0); > } > > -static void membw_ioctl_perf_event_ioc_disable(int i, int j) > +static void read_mem_bw_ioctl_perf_event_ioc_disable(int i) > { > - ioctl(imc_counters_config[i][j].fd, PERF_EVENT_IOC_DISABLE, 0); > + ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_DISABLE, 0); > } > > /* > - * get_event_and_umask: Parse config into event and umask > + * get_read_event_and_umask: Parse config into event and umask > * @cas_count_cfg: Config > * @count: iMC number > - * @op: Operation (read/write) > */ > -static void get_event_and_umask(char *cas_count_cfg, int count, bool op) > +static void get_read_event_and_umask(char *cas_count_cfg, int count) > { > char *token[MAX_TOKENS]; > int i = 0; > @@ -91,34 +87,22 @@ static void get_event_and_umask(char *cas_count_cfg, int count, bool op) > for (i = 0; i < MAX_TOKENS - 1; i++) { > if (!token[i]) > break; > - if (strcmp(token[i], "event") == 0) { > - if (op == READ) > - imc_counters_config[count][READ].event = > - strtol(token[i + 1], NULL, 16); > - else > - imc_counters_config[count][WRITE].event = > - strtol(token[i + 1], NULL, 16); > - } > - if (strcmp(token[i], "umask") == 0) { > - if (op == READ) > - imc_counters_config[count][READ].umask = > - strtol(token[i + 1], NULL, 16); > - else > - imc_counters_config[count][WRITE].umask = > - strtol(token[i + 1], NULL, 16); > - } > + if (strcmp(token[i], "event") == 0) > + imc_counters_config[count].event = strtol(token[i + 1], NULL, 16); > + if (strcmp(token[i], "umask") == 0) > + imc_counters_config[count].umask = strtol(token[i + 1], NULL, 16); > } > } > > -static int open_perf_event(int i, int cpu_no, int j) > +static int open_perf_read_event(int i, int cpu_no) > { > - imc_counters_config[i][j].fd = > - perf_event_open(&imc_counters_config[i][j].pe, -1, cpu_no, -1, > + imc_counters_config[i].fd = > + perf_event_open(&imc_counters_config[i].pe, -1, cpu_no, -1, > PERF_FLAG_FD_CLOEXEC); > > - if (imc_counters_config[i][j].fd == -1) { > + if (imc_counters_config[i].fd == -1) { > fprintf(stderr, "Error opening leader %llx\n", > - imc_counters_config[i][j].pe.config); > + imc_counters_config[i].pe.config); > > return -1; > } > @@ -126,7 +110,7 @@ static int open_perf_event(int i, int cpu_no, int j) > return 0; > } > > -/* Get type and config (read and write) of an iMC counter */ > +/* Get type and config of an iMC counter's read event. */ > static int read_from_imc_dir(char *imc_dir, int count) > { > char cas_count_cfg[1024], imc_counter_cfg[1024], imc_counter_type[1024]; > @@ -140,7 +124,7 @@ static int read_from_imc_dir(char *imc_dir, int count) > > return -1; > } > - if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { > + if (fscanf(fp, "%u", &imc_counters_config[count].type) <= 0) { > ksft_perror("Could not get iMC type"); > fclose(fp); > > @@ -148,9 +132,6 @@ static int read_from_imc_dir(char *imc_dir, int count) > } > fclose(fp); > > - imc_counters_config[count][WRITE].type = > - imc_counters_config[count][READ].type; > - > /* Get read config */ > sprintf(imc_counter_cfg, "%s%s", imc_dir, READ_FILE_NAME); > fp = fopen(imc_counter_cfg, "r"); > @@ -167,34 +148,19 @@ static int read_from_imc_dir(char *imc_dir, int count) > } > fclose(fp); > > - get_event_and_umask(cas_count_cfg, count, READ); > - > - /* Get write config */ > - sprintf(imc_counter_cfg, "%s%s", imc_dir, WRITE_FILE_NAME); > - fp = fopen(imc_counter_cfg, "r"); > - if (!fp) { > - ksft_perror("Failed to open iMC config file"); > - > - return -1; > - } > - if (fscanf(fp, "%s", cas_count_cfg) <= 0) { > - ksft_perror("Could not get iMC cas count write"); > - fclose(fp); > - > - return -1; > - } > - fclose(fp); > - > - get_event_and_umask(cas_count_cfg, count, WRITE); > + get_read_event_and_umask(cas_count_cfg, count); > > return 0; > } > > /* > * A system can have 'n' number of iMC (Integrated Memory Controller) > - * counters, get that 'n'. For each iMC counter get it's type and config. > - * Also, each counter has two configs, one for read and the other for write. > - * A config again has two parts, event and umask. > + * counters, get that 'n'. Discover the properties of the available > + * counters in support of needed performance measurement via perf. > + * For each iMC counter get it's type and config. Also obtain each > + * counter's event and umask for the memory read events that will be > + * measured. > + * > * Enumerate all these details into an array of structures. > * > * Return: >= 0 on success. < 0 on failure. > @@ -255,55 +221,46 @@ static int num_of_imcs(void) > return count; > } > > -int initialize_mem_bw_imc(void) > +int initialize_read_mem_bw_imc(void) > { > - int imc, j; > + int imc; > > imcs = num_of_imcs(); > if (imcs <= 0) > return imcs; > > /* Initialize perf_event_attr structures for all iMC's */ > - for (imc = 0; imc < imcs; imc++) { > - for (j = 0; j < 2; j++) > - membw_initialize_perf_event_attr(imc, j); > - } > + for (imc = 0; imc < imcs; imc++) > + read_mem_bw_initialize_perf_event_attr(imc); > > return 0; > } > > -static void perf_close_imc_mem_bw(void) > +static void perf_close_imc_read_mem_bw(void) > { > int mc; > > for (mc = 0; mc < imcs; mc++) { > - if (imc_counters_config[mc][READ].fd != -1) > - close(imc_counters_config[mc][READ].fd); > - if (imc_counters_config[mc][WRITE].fd != -1) > - close(imc_counters_config[mc][WRITE].fd); > + if (imc_counters_config[mc].fd != -1) > + close(imc_counters_config[mc].fd); > } > } > > /* > - * perf_open_imc_mem_bw - Open perf fds for IMCs > + * perf_open_imc_read_mem_bw - Open perf fds for IMCs > * @cpu_no: CPU number that the benchmark PID is bound to > * > * Return: = 0 on success. < 0 on failure. > */ > -static int perf_open_imc_mem_bw(int cpu_no) > +static int perf_open_imc_read_mem_bw(int cpu_no) > { > int imc, ret; > > - for (imc = 0; imc < imcs; imc++) { > - imc_counters_config[imc][READ].fd = -1; > - imc_counters_config[imc][WRITE].fd = -1; > - } > + for (imc = 0; imc < imcs; imc++) > + imc_counters_config[imc].fd = -1; > > for (imc = 0; imc < imcs; imc++) { > - ret = open_perf_event(imc, cpu_no, READ); > - if (ret) > - goto close_fds; > - ret = open_perf_event(imc, cpu_no, WRITE); > + ret = open_perf_read_event(imc, cpu_no); > if (ret) > goto close_fds; > } > @@ -311,60 +268,52 @@ static int perf_open_imc_mem_bw(int cpu_no) > return 0; > > close_fds: > - perf_close_imc_mem_bw(); > + perf_close_imc_read_mem_bw(); > return -1; > } > > /* > - * do_mem_bw_test - Perform memory bandwidth test > + * do_imc_read_mem_bw_test - Perform memory bandwidth test > * > * Runs memory bandwidth test over one second period. Also, handles starting > * and stopping of the IMC perf counters around the test. > */ > -static void do_imc_mem_bw_test(void) > +static void do_imc_read_mem_bw_test(void) > { > int imc; > > - for (imc = 0; imc < imcs; imc++) { > - membw_ioctl_perf_event_ioc_reset_enable(imc, READ); > - membw_ioctl_perf_event_ioc_reset_enable(imc, WRITE); > - } > + for (imc = 0; imc < imcs; imc++) > + read_mem_bw_ioctl_perf_event_ioc_reset_enable(imc); > > sleep(1); > > - /* Stop counters after a second to get results (both read and write) */ > - for (imc = 0; imc < imcs; imc++) { > - membw_ioctl_perf_event_ioc_disable(imc, READ); > - membw_ioctl_perf_event_ioc_disable(imc, WRITE); > - } > + /* Stop counters after a second to get results. */ > + for (imc = 0; imc < imcs; imc++) > + read_mem_bw_ioctl_perf_event_ioc_disable(imc); > } > > /* > - * get_mem_bw_imc - Memory bandwidth as reported by iMC counters > - * @bw_report: Bandwidth report type (reads, writes) > + * get_read_mem_bw_imc - Memory read bandwidth as reported by iMC counters > * > - * Memory bandwidth utilized by a process on a socket can be calculated > - * using iMC counters. Perf events are used to read these counters. > + * Memory read bandwidth utilized by a process on a socket can be calculated > + * using iMC counters' read events. Perf events are used to read these > + * counters. > * > * Return: = 0 on success. < 0 on failure. > */ > -static int get_mem_bw_imc(const char *bw_report, float *bw_imc) > +static int get_read_mem_bw_imc(float *bw_imc) > { > - float reads, writes, of_mul_read, of_mul_write; > + float reads = 0, of_mul_read = 1; > int imc; > > - /* Start all iMC counters to log values (both read and write) */ > - reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1; > - > /* > - * Get results which are stored in struct type imc_counter_config > + * Log read event values from all iMC counters into > + * struct imc_counter_config. > * Take overflow into consideration before calculating total bandwidth. > */ > for (imc = 0; imc < imcs; imc++) { > struct imc_counter_config *r = > - &imc_counters_config[imc][READ]; > - struct imc_counter_config *w = > - &imc_counters_config[imc][WRITE]; > + &imc_counters_config[imc]; > > if (read(r->fd, &r->return_value, > sizeof(struct membw_read_format)) == -1) { > @@ -372,12 +321,6 @@ static int get_mem_bw_imc(const char *bw_report, float *bw_imc) > return -1; > } > > - if (read(w->fd, &w->return_value, > - sizeof(struct membw_read_format)) == -1) { > - ksft_perror("Couldn't get write bandwidth through iMC"); > - return -1; > - } > - > __u64 r_time_enabled = r->return_value.time_enabled; > __u64 r_time_running = r->return_value.time_running; > > @@ -385,27 +328,10 @@ static int get_mem_bw_imc(const char *bw_report, float *bw_imc) > of_mul_read = (float)r_time_enabled / > (float)r_time_running; > > - __u64 w_time_enabled = w->return_value.time_enabled; > - __u64 w_time_running = w->return_value.time_running; > - > - if (w_time_enabled != w_time_running) > - of_mul_write = (float)w_time_enabled / > - (float)w_time_running; > reads += r->return_value.value * of_mul_read * SCALE; > - writes += w->return_value.value * of_mul_write * SCALE; > } > > - if (strcmp(bw_report, "reads") == 0) { > - *bw_imc = reads; > - return 0; > - } > - > - if (strcmp(bw_report, "writes") == 0) { > - *bw_imc = writes; > - return 0; > - } > - > - *bw_imc = reads + writes; > + *bw_imc = reads; > return 0; > } > > @@ -551,35 +477,31 @@ static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc, > } > > /* > - * measure_mem_bw - Measures memory bandwidth numbers while benchmark runs > + * measure_read_mem_bw - Measures read memory bandwidth numbers while benchmark runs > * @uparams: User supplied parameters > * @param: Parameters passed to resctrl_val() > * @bm_pid: PID that runs the benchmark > - * @bw_report: Bandwidth report type (reads, writes) > * > * Measure memory bandwidth from resctrl and from another source which is > * perf imc value or could be something else if perf imc event is not > * available. Compare the two values to validate resctrl value. It takes > * 1 sec to measure the data. > + * resctrl does not distinguish between read and write operations so > + * its data includes all memory operations. > */ > -int measure_mem_bw(const struct user_params *uparams, > - struct resctrl_val_param *param, pid_t bm_pid, > - const char *bw_report) > +int measure_read_mem_bw(const struct user_params *uparams, > + struct resctrl_val_param *param, pid_t bm_pid) > { > unsigned long bw_resc, bw_resc_start, bw_resc_end; > FILE *mem_bw_fp; > float bw_imc; > int ret; > > - bw_report = get_bw_report_type(bw_report); > - if (!bw_report) > - return -1; > - > mem_bw_fp = open_mem_bw_resctrl(mbm_total_path); > if (!mem_bw_fp) > return -1; > > - ret = perf_open_imc_mem_bw(uparams->cpu); > + ret = perf_open_imc_read_mem_bw(uparams->cpu); > if (ret < 0) > goto close_fp; > > @@ -589,17 +511,17 @@ int measure_mem_bw(const struct user_params *uparams, > > rewind(mem_bw_fp); > > - do_imc_mem_bw_test(); > + do_imc_read_mem_bw_test(); > > ret = get_mem_bw_resctrl(mem_bw_fp, &bw_resc_end); > if (ret < 0) > goto close_imc; > > - ret = get_mem_bw_imc(bw_report, &bw_imc); > + ret = get_read_mem_bw_imc(&bw_imc); > if (ret < 0) > goto close_imc; > > - perf_close_imc_mem_bw(); > + perf_close_imc_read_mem_bw(); > fclose(mem_bw_fp); > > bw_resc = (bw_resc_end - bw_resc_start) / MB; > @@ -607,7 +529,7 @@ int measure_mem_bw(const struct user_params *uparams, > return print_results_bw(param->filename, bm_pid, bw_imc, bw_resc); > > close_imc: > - perf_close_imc_mem_bw(); > + perf_close_imc_read_mem_bw(); > close_fp: > fclose(mem_bw_fp); > return ret; > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > index 250c320349a7..eabdf0c20566 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -831,23 +831,6 @@ int filter_dmesg(void) > return 0; > } > > -const char *get_bw_report_type(const char *bw_report) > -{ > - if (strcmp(bw_report, "reads") == 0) > - return bw_report; > - if (strcmp(bw_report, "writes") == 0) > - return bw_report; > - if (strcmp(bw_report, "nt-writes") == 0) { > - return "writes"; > - } > - if (strcmp(bw_report, "total") == 0) > - return bw_report; > - > - fprintf(stderr, "Requested iMC bandwidth report type unavailable\n"); > - > - return NULL; > -} > - > int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu, > int group_fd, unsigned long flags) > { >