From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Waiman Long <longman@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@kernel.org>, Will Deacon <will@kernel.org>,
Boqun Feng <boqun.feng@gmail.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
rostedt <rostedt@goodmis.org>,
Byungchul Park <byungchul.park@lge.com>,
Radoslaw Burny <rburny@google.com>, Tejun Heo <tj@kernel.org>,
rcu <rcu@vger.kernel.org>, cgroups <cgroups@vger.kernel.org>,
linux-btrfs <linux-btrfs@vger.kernel.org>,
intel-gfx <intel-gfx@lists.freedesktop.org>,
paulmck <paulmck@kernel.org>
Subject: Re: [RFC 00/12] locking: Separate lock tracepoints from lockdep/lock_stat (v1)
Date: Wed, 9 Feb 2022 14:28:10 -0500 (EST) [thread overview]
Message-ID: <718973621.50447.1644434890744.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <CAM9d7ci=N2NVj57k=W0ebqBzfW+ThBqYSrx-CZbgwGcbOSrEGA@mail.gmail.com>
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung@kernel.org wrote:
> Hello,
>
> On Wed, Feb 9, 2022 at 11:02 AM Waiman Long <longman@redhat.com> wrote:
>>
>> On 2/9/22 13:29, Mathieu Desnoyers wrote:
>> > ----- On Feb 9, 2022, at 1:19 PM, Waiman Long longman@redhat.com wrote:
>> >
>> >> On 2/9/22 04:09, Peter Zijlstra wrote:
>> >>> On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
>> >>>
>> >>>> Eventually I'm mostly interested in the contended locks only and I
>> >>>> want to reduce the overhead in the fast path. By moving that, it'd be
>> >>>> easy to track contended locks with timing by using two tracepoints.
>> >>> So why not put in two new tracepoints and call it a day?
>> >>>
>> >>> Why muck about with all that lockdep stuff just to preserve the name
>> >>> (and in the process continue to blow up data structures etc..). This
>> >>> leaves distros in a bind, will they enable this config and provide
>> >>> tracepoints while bloating the data structures and destroying things
>> >>> like lockref (which relies on sizeof(spinlock_t)), or not provide this
>> >>> at all.
>> >>>
>> >>> Yes, the name is convenient, but it's just not worth it IMO. It makes
>> >>> the whole proposition too much of a trade-off.
>> >>>
>> >>> Would it not be possible to reconstruct enough useful information from
>> >>> the lock callsite?
>> >>>
>> >> I second that as I don't want to see the size of a spinlock exceeds 4
>> >> bytes in a production system.
>> >>
>> >> Instead of storing additional information (e.g. lock name) directly into
>> >> the lock itself. Maybe we can store it elsewhere and use the lock
>> >> address as the key to locate it in a hash table. We can certainly extend
>> >> the various lock init functions to do that. It will be trickier for
>> >> statically initialized locks, but we can probably find a way to do that too.
>> > If we go down that route, it would be nice if we can support a few different
>> > use-cases for various tracers out there.
>> >
>> > One use-case (a) requires the ability to query the lock name based on its
>> > address as key.
>> > For this a hash table is a good fit. This would allow tracers like ftrace to
>> > output lock names in its human-readable output which is formatted within the
>> > kernel.
>> >
>> > Another use-case (b) is to be able to "dump" the lock { name, address } tuples
>> > into the trace stream (we call this statedump events in lttng), and do the
>> > translation from address to name at post-processing. This simply requires
>> > that this information is available for iteration for both the core kernel
>> > and module locks, so the tracer can dump this information on trace start
>> > and module load.
>> >
>> > Use-case (b) is very similar to what is done for the kernel tracepoints. Based
>> > on this, implementing the init code that iterates on those sections and
>> > populates
>> > a hash table for use-case (a) should be easy enough.
>>
>> Yes, that are good use cases for this type of functionality. I do need
>> to think about how to do it for statically initialized lock first.
>
> Thank you all for the review and good suggestions.
>
> I'm also concerning dynamic allocated locks in a data structure.
> If we keep the info in a hash table, we should delete it when the
> lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
But perhaps I am missing something here.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
next prev parent reply other threads:[~2022-02-09 19:29 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-08 18:41 [RFC 00/12] locking: Separate lock tracepoints from lockdep/lock_stat (v1) Namhyung Kim
2022-02-08 18:41 ` [PATCH 01/12] locking: Pass correct outer wait type info Namhyung Kim
2022-02-08 18:41 ` [PATCH 02/12] cgroup: rstat: Make cgroup_rstat_cpu_lock name readable Namhyung Kim
2022-02-08 18:46 ` Tejun Heo
2022-02-08 19:16 ` Namhyung Kim
2022-02-08 23:51 ` Namhyung Kim
2022-02-08 18:41 ` [PATCH 03/12] timer: Protect lockdep functions with #ifdef Namhyung Kim
2022-02-08 19:36 ` Steven Rostedt
2022-02-08 20:29 ` Namhyung Kim
2022-02-08 21:19 ` Steven Rostedt
2022-02-08 18:42 ` [PATCH 04/12] workqueue: " Namhyung Kim
2022-02-08 18:48 ` Tejun Heo
2022-02-08 19:17 ` Namhyung Kim
2022-02-08 19:38 ` Steven Rostedt
2022-02-08 18:42 ` [PATCH 05/12] drm/i915: " Namhyung Kim
2022-02-08 18:51 ` Jani Nikula
2022-02-08 19:22 ` Namhyung Kim
2022-02-09 13:49 ` Jani Nikula
2022-02-09 16:27 ` Steven Rostedt
2022-02-09 19:28 ` Namhyung Kim
2022-02-08 18:42 ` [PATCH 06/12] btrfs: change lockdep class size check using ks->names Namhyung Kim
2022-02-08 19:03 ` David Sterba
2022-02-08 18:42 ` [PATCH 07/12] locking: Introduce CONFIG_LOCK_INFO Namhyung Kim
2022-02-08 18:42 ` [PATCH 08/12] locking/mutex: Init name properly w/ CONFIG_LOCK_INFO Namhyung Kim
2022-02-08 18:42 ` [PATCH 09/12] locking: Add more static lockdep init macros Namhyung Kim
2022-02-08 18:42 ` [PATCH 10/12] locking: Add CONFIG_LOCK_TRACEPOINTS option Namhyung Kim
2022-02-08 18:42 ` [PATCH 11/12] locking/mutex: Revive fast functions for CONFIG_LOCK_TRACEPOINTS Namhyung Kim
2022-02-09 8:40 ` Peter Zijlstra
2022-02-09 20:15 ` Namhyung Kim
2022-02-08 18:42 ` [PATCH 12/12] locking: Move lock_acquired() from the fast path Namhyung Kim
2022-02-08 19:14 ` [RFC 00/12] locking: Separate lock tracepoints from lockdep/lock_stat (v1) Namhyung Kim
2022-02-09 9:09 ` Peter Zijlstra
2022-02-09 18:19 ` Waiman Long
2022-02-09 18:29 ` Mathieu Desnoyers
2022-02-09 19:02 ` Waiman Long
2022-02-09 19:17 ` Mathieu Desnoyers
2022-02-09 19:37 ` Waiman Long
2022-02-09 19:22 ` Namhyung Kim
2022-02-09 19:28 ` Mathieu Desnoyers [this message]
2022-02-09 19:45 ` Namhyung Kim
2022-02-09 19:56 ` Mathieu Desnoyers
2022-02-09 20:17 ` Waiman Long
2022-02-10 0:27 ` Namhyung Kim
2022-02-10 2:12 ` Waiman Long
2022-02-10 9:33 ` Peter Zijlstra
2022-02-10 0:32 ` Namhyung Kim
2022-02-10 9:13 ` Peter Zijlstra
2022-02-10 19:14 ` Paul E. McKenney
2022-02-10 19:27 ` Waiman Long
2022-02-10 20:10 ` Paul E. McKenney
2022-02-11 5:57 ` Namhyung Kim
2022-02-11 5:55 ` Namhyung Kim
2022-02-11 10:39 ` Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=718973621.50447.1644434890744.JavaMail.zimbra@efficios.com \
--to=mathieu.desnoyers@efficios.com \
--cc=boqun.feng@gmail.com \
--cc=byungchul.park@lge.com \
--cc=cgroups@vger.kernel.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=longman@redhat.com \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rburny@google.com \
--cc=rcu@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=tj@kernel.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).