lkml.org 
[lkml]   [2023]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 10/24] selftests/resctrl: Remove nested calls in perf event handling
    From
    Hi Ilpo,

    On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
    > Perf event handling has functions that are the sole caller of another
    > perf event handling related function:
    > - reset_enable_llc_perf() calls perf_event_open_llc_miss()
    > - reset_enable_llc_perf() calls ioctl_perf_event_ioc_reset_enable()
    > - measure_llc_perf() calls get_llc_perf()
    >
    > Remove the extra layer of calls to make the code easier to follow by
    > moving the code into the calling function.
    >
    > In addition, converts print_results_cache() unsigned long parameter to
    > __u64 that matches the type coming from perf.

    Is this referring to work from previous patch?

    >
    > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
    > ---
    > tools/testing/selftests/resctrl/cache.c | 86 +++++++------------------
    > 1 file changed, 25 insertions(+), 61 deletions(-)
    >
    > diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
    > index d39ef4eebc37..208af1ecae28 100644
    > --- a/tools/testing/selftests/resctrl/cache.c
    > +++ b/tools/testing/selftests/resctrl/cache.c
    > @@ -29,25 +29,6 @@ static void initialize_perf_event_attr(void)
    > pea_llc_miss.disabled = 1;
    > }
    >
    > -static void ioctl_perf_event_ioc_reset_enable(void)
    > -{
    > - ioctl(fd_lm, PERF_EVENT_IOC_RESET, 0);
    > - ioctl(fd_lm, PERF_EVENT_IOC_ENABLE, 0);
    > -}
    > -
    > -static int perf_event_open_llc_miss(pid_t pid, int cpu_no)
    > -{
    > - fd_lm = perf_event_open(&pea_llc_miss, pid, cpu_no, -1,
    > - PERF_FLAG_FD_CLOEXEC);
    > - if (fd_lm == -1) {
    > - perror("Error opening leader");
    > - ctrlc_handler(0, NULL, NULL);
    > - return -1;
    > - }
    > -
    > - return 0;
    > -}
    > -
    > static void initialize_llc_perf(void)
    > {
    > memset(&pea_llc_miss, 0, sizeof(struct perf_event_attr));
    > @@ -63,42 +44,16 @@ static void initialize_llc_perf(void)
    >
    > static int reset_enable_llc_perf(pid_t pid, int cpu_no)
    > {
    > - int ret = 0;
    > -
    > - ret = perf_event_open_llc_miss(pid, cpu_no);
    > - if (ret < 0)
    > - return ret;
    > -
    > - /* Start counters to log values */
    > - ioctl_perf_event_ioc_reset_enable();
    > -
    > - return 0;
    > -}
    > -
    > -/*
    > - * get_llc_perf: llc cache miss through perf events
    > - * @llc_perf_miss: LLC miss counter that is filled on success
    > - *
    > - * Perf events like HW_CACHE_MISSES could be used to validate number of
    > - * cache lines allocated.
    > - *
    > - * Return: =0 on success. <0 on failure.
    > - */
    > -static int get_llc_perf(__u64 *llc_perf_miss)
    > -{
    > - int ret;
    > -
    > - /* Stop counters after one span to get miss rate */
    > -
    > - ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
    > -
    > - ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
    > - if (ret == -1) {
    > - perror("Could not get llc misses through perf");
    > + fd_lm = perf_event_open(&pea_llc_miss, pid, cpu_no, -1, PERF_FLAG_FD_CLOEXEC);
    > + if (fd_lm == -1) {
    > + perror("Error opening leader");
    > + ctrlc_handler(0, NULL, NULL);

    I understand you just copied the code here ... but it is not clear to me
    why this particular error handling deserves a ctrlc_handler().

    > return -1;
    > }
    >
    > - *llc_perf_miss = rf_cqm.values[0].value;
    > + /* Start counters to log values */
    > + ioctl(fd_lm, PERF_EVENT_IOC_RESET, 0);
    > + ioctl(fd_lm, PERF_EVENT_IOC_ENABLE, 0);
    >
    > return 0;
    > }
    > @@ -166,20 +121,29 @@ static int print_results_cache(char *filename, int bm_pid, __u64 llc_value)
    > return 0;
    > }
    >
    > +/*
    > + * measure_llc_perf: measure perf events
    > + * @bm_pid: child pid that runs benchmark

    I expected "bm_pid" to reflect a "benchmark pid" that
    is not unique to the child. Are both parent and child
    not running the benchmark?

    Missing doc of a parameter here.

    > + *
    > + * Measure things like cache misses from perf events.

    "things like cache misses" is vague. The function's name
    still contains "llc" which makes me think it is not quite
    generic yet.



    > + *
    > + * Return: =0 on success. <0 on failure.
    > + */
    > static int measure_llc_perf(struct resctrl_val_param *param, int bm_pid)
    > {
    > - __u64 llc_perf_miss = 0;
    > int ret;
    >
    > - /*
    > - * Measure cache miss from perf.
    > - */
    > - ret = get_llc_perf(&llc_perf_miss);
    > - if (ret < 0)
    > - return ret;
    > + /* Stop counters after one span to get miss rate */
    > + ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
    >
    > - ret = print_results_cache(param->filename, bm_pid, llc_perf_miss);
    > - return ret;
    > + ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
    > + close(fd_lm);

    I am not able to tell where this close() moved from.

    > + if (ret == -1) {
    > + perror("Could not get perf value");
    > + return -1;
    > + }
    > +
    > + return print_results_cache(param->filename, bm_pid, rf_cqm.values[0].value);
    > }
    >
    > int measure_llc_resctrl(struct resctrl_val_param *param, int bm_pid)

    Reinette

    \
     
     \ /
      Last update: 2023-11-02 18:50    [W:2.269 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site