On Thu, 12 Sep 2024, Reinette Chatre wrote:
> The MBM and MBA resctrl selftests run a benchmark during which
> it takes measurements of read memory bandwidth via perf.
> Code exists to support measurements of write memory bandwidth
> but there exists no path with which this code can execute.
>
> While code exists for write memory bandwidth measurement
> there has not yet been a use case for it. Remove this unused code.
> Rename relevant functions to include "read" so that it is clear
> that it relates only to memory bandwidth reads, while renaming
> the functions also add consistency by changing the "membw"
> instances to more prevalent "mem_bw".
>
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
I'll take a look at the rest of the series later.
--
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)
> {
>
