linux-kernel - Re: [PATCH v5 1/3] perf lock: Fix parse_lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAM9d7cgWLVqUhf91Z4tNwDt-4LQevB0Qhhad6P-FXRfh0-d8PA@mail.gmail.com>
Date: Thu, 16 Jan 2025 15:41:53 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Chun-Tse Shao <ctshao@...gle.com>
Cc: linux-kernel@...r.kernel.org, peterz@...radead.org, mingo@...hat.com, 
	acme@...nel.org, mark.rutland@....com, alexander.shishkin@...ux.intel.com, 
	jolsa@...nel.org, irogers@...gle.com, adrian.hunter@...el.com, 
	kan.liang@...ux.intel.com, nick.forrington@....com, 
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v5 1/3] perf lock: Fix parse_lock_type which only retrieve
 one lock flag

On Wed, Dec 11, 2024 at 11:17 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Tue, Dec 10, 2024 at 12:08:20PM -0800, Chun-Tse Shao wrote:
> > `parse_lock_type` can only add the first lock flag in `lock_type_table`
> > given input `str`. For example, for `Y rwlock`, it only adds `rwlock:R`
> > into this perf session. Another example is for `-Y mutex`, it only adds
> > the mutex without `LCB_F_SPIN` flag. The patch fixes this issue, makes
> > sure both `rwlock:R` and `rwlock:W` will be added with `-Y rwlock`, and
> > so on.
> >
> > Testing:
> >   $ ./perf lock con -ab -Y mutex,rwlock -- perf bench sched pipe
> >   # Running 'sched/pipe' benchmark:
> >   # Executed 1000000 pipe operations between two processes
> >
> >        Total time: 9.313 [sec]
> >
> >          9.313976 usecs/op
> >            107365 ops/sec
> >    contended   total wait     max wait     avg wait         type   caller
> >
> >          176      1.65 ms     19.43 us      9.38 us        mutex   pipe_read+0x57
> >           34    180.14 us     10.93 us      5.30 us        mutex   pipe_write+0x50
> >            7     77.48 us     16.09 us     11.07 us        mutex   do_epoll_wait+0x24d
> >            7     74.70 us     13.50 us     10.67 us        mutex   do_epoll_wait+0x24d
> >            3     35.97 us     14.44 us     11.99 us     rwlock:W   ep_done_scan+0x2d
> >            3     35.00 us     12.23 us     11.66 us     rwlock:W   do_epoll_wait+0x255
> >            2     15.88 us     11.96 us      7.94 us     rwlock:W   do_epoll_wait+0x47c
> >            1     15.23 us     15.23 us     15.23 us     rwlock:W   do_epoll_wait+0x4d0
> >            1     14.26 us     14.26 us     14.26 us     rwlock:W   ep_done_scan+0x2d
> >            2     14.00 us      7.99 us      7.00 us        mutex   pipe_read+0x282
> >            1     12.29 us     12.29 us     12.29 us     rwlock:R   ep_poll_callback+0x35
> >            1     12.02 us     12.02 us     12.02 us     rwlock:W   do_epoll_ctl+0xb65
> >            1     10.25 us     10.25 us     10.25 us     rwlock:R   ep_poll_callback+0x35
> >            1      7.86 us      7.86 us      7.86 us        mutex   do_epoll_ctl+0x6c1
> >            1      5.04 us      5.04 us      5.04 us        mutex   do_epoll_ctl+0x3d4
> >
> > Fixes: d783ea8f62c4 ("perf lock contention: Simplify parse_lock_type()")
> > Signed-off-by: Chun-Tse Shao <ctshao@...gle.com>
>
> Reviewed-by: Namhyung Kim <namhyung@...nel.org>
>
> Thanks,
> Namhyung
>
> > ---
> >  tools/perf/builtin-lock.c | 64 ++++++++++++++++++++++++---------------
> >  1 file changed, 39 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index 062e2b56a2ab..7e36bbe3cb80 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -1591,8 +1591,6 @@ static const struct {
> >       { LCB_F_PERCPU | LCB_F_WRITE,   "pcpu-sem:W",   "percpu-rwsem" },
> >       { LCB_F_MUTEX,                  "mutex",        "mutex" },
> >       { LCB_F_MUTEX | LCB_F_SPIN,     "mutex",        "mutex" },
> > -     /* alias for get_type_flag() */
> > -     { LCB_F_MUTEX | LCB_F_SPIN,     "mutex-spin",   "mutex" },

Hmm.. now I think we need this part to keep it compatible.
Let's have "mutex-spin" in the 'name' as well.

Thanks,
Namhyung