From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Dave Jones <davej@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Alan Stern <stern@rowland.harvard.edu>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Ingo Molnar <mingo@kernel.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Steven Rostedt <rostedt@goodmis.org>,
Thomas Gleixner <tglx@linutronix.de>,
linux-kernel@vger.kernel.org
Subject: Re: uninline rcu_lock_acquire/etc ?
Date: Tue, 21 Jan 2014 19:54:40 -0800 [thread overview]
Message-ID: <20140122035440.GW10038@linux.vnet.ibm.com> (raw)
In-Reply-To: <20140121193909.GA17497@redhat.com>
On Tue, Jan 21, 2014 at 08:39:09PM +0100, Oleg Nesterov wrote:
> On 01/21, Oleg Nesterov wrote:
> >
> > But I agreed that the code looks simpler with bitfields, so perhaps
> > this patch is better.
>
> Besides, I guess the major offender is rcu...
>
> Paul, can't we do something like below? Saves 19.5 kilobytes,
>
> - 5255131 2974376 10125312 18354819 1181283 vmlinux
> + 5235227 2970344 10125312 18330883 117b503 vmlinux
>
> probably we can also uninline rcu_lockdep_assert()...
Looks mostly plausible, some questions inline below.
Thanx, Paul
> Oleg.
> ---
>
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 2eef290..58f7a97 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -310,18 +310,34 @@ static inline bool rcu_lockdep_current_cpu_online(void)
> }
> #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
>
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> -
> -static inline void rcu_lock_acquire(struct lockdep_map *map)
> +static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
> {
> - lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
> + lock_acquire(map, 0, 0, 2, 0, NULL, ip);
> }
>
> -static inline void rcu_lock_release(struct lockdep_map *map)
> +static inline void __rcu_lock_release(struct lockdep_map *map, unsigned long ip)
> {
> lock_release(map, 1, _THIS_IP_);
> }
>
> +#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
> +extern void rcu_lock_acquire(void);
> +extern void rcu_lock_release(void);
> +extern void rcu_lock_acquire_bh(void);
> +extern void rcu_lock_release_bh(void);
> +extern void rcu_lock_acquire_sched(void);
> +extern void rcu_lock_release_sched(void);
> +#else
> +#define rcu_lock_acquire() do { } while (0)
> +#define rcu_lock_release() do { } while (0)
> +#define rcu_lock_acquire_bh() do { } while (0)
> +#define rcu_lock_release_bh() do { } while (0)
> +#define rcu_lock_acquire_sched() do { } while (0)
> +#define rcu_lock_release_sched() do { } while (0)
> +#endif
> +
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +
> extern struct lockdep_map rcu_lock_map;
> extern struct lockdep_map rcu_bh_lock_map;
> extern struct lockdep_map rcu_sched_lock_map;
> @@ -419,9 +435,6 @@ static inline int rcu_read_lock_sched_held(void)
>
> #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> -# define rcu_lock_acquire(a) do { } while (0)
> -# define rcu_lock_release(a) do { } while (0)
> -
> static inline int rcu_read_lock_held(void)
> {
> return 1;
> @@ -766,11 +779,9 @@ static inline void rcu_preempt_sleep_check(void)
> */
> static inline void rcu_read_lock(void)
> {
> - __rcu_read_lock();
> __acquire(RCU);
> - rcu_lock_acquire(&rcu_lock_map);
> - rcu_lockdep_assert(rcu_is_watching(),
> - "rcu_read_lock() used illegally while idle");
> + __rcu_read_lock();
> + rcu_lock_acquire();
Not sure why __rcu_read_lock() needs to be in any particular order
with respect to the sparse __acquire(RCU), but should work either way.
Same question about the other reorderings of similar statements.
> }
>
> /*
> @@ -790,11 +801,9 @@ static inline void rcu_read_lock(void)
> */
> static inline void rcu_read_unlock(void)
> {
> - rcu_lockdep_assert(rcu_is_watching(),
> - "rcu_read_unlock() used illegally while idle");
> - rcu_lock_release(&rcu_lock_map);
> - __release(RCU);
> + rcu_lock_release();
> __rcu_read_unlock();
> + __release(RCU);
> }
>
> /**
> @@ -816,11 +825,9 @@ static inline void rcu_read_unlock(void)
> */
> static inline void rcu_read_lock_bh(void)
> {
> - local_bh_disable();
> __acquire(RCU_BH);
> - rcu_lock_acquire(&rcu_bh_lock_map);
> - rcu_lockdep_assert(rcu_is_watching(),
> - "rcu_read_lock_bh() used illegally while idle");
> + local_bh_disable();
> + rcu_lock_acquire_bh();
> }
>
> /*
> @@ -830,11 +837,9 @@ static inline void rcu_read_lock_bh(void)
> */
> static inline void rcu_read_unlock_bh(void)
> {
> - rcu_lockdep_assert(rcu_is_watching(),
> - "rcu_read_unlock_bh() used illegally while idle");
> - rcu_lock_release(&rcu_bh_lock_map);
> - __release(RCU_BH);
> + rcu_lock_release_bh();
> local_bh_enable();
> + __release(RCU_BH);
> }
>
> /**
> @@ -852,9 +857,9 @@ static inline void rcu_read_unlock_bh(void)
> */
> static inline void rcu_read_lock_sched(void)
> {
> - preempt_disable();
> __acquire(RCU_SCHED);
> - rcu_lock_acquire(&rcu_sched_lock_map);
> + preempt_disable();
> + rcu_lock_acquire_sched();
> rcu_lockdep_assert(rcu_is_watching(),
> "rcu_read_lock_sched() used illegally while idle");
The above pair of lines (rcu_lockdep_assert()) should also be removed,
correct?
> }
> @@ -862,8 +867,8 @@ static inline void rcu_read_lock_sched(void)
> /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */
> static inline notrace void rcu_read_lock_sched_notrace(void)
> {
> - preempt_disable_notrace();
> __acquire(RCU_SCHED);
> + preempt_disable_notrace();
I cannot help repeating myself on this one... ;-)
Why the change in order?
> }
>
> /*
> @@ -873,18 +878,16 @@ static inline notrace void rcu_read_lock_sched_notrace(void)
> */
> static inline void rcu_read_unlock_sched(void)
> {
> - rcu_lockdep_assert(rcu_is_watching(),
> - "rcu_read_unlock_sched() used illegally while idle");
> - rcu_lock_release(&rcu_sched_lock_map);
> - __release(RCU_SCHED);
> + rcu_lock_release_sched();
> preempt_enable();
> + __release(RCU_SCHED);
> }
>
> /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */
> static inline notrace void rcu_read_unlock_sched_notrace(void)
> {
> - __release(RCU_SCHED);
> preempt_enable_notrace();
> + __release(RCU_SCHED);
> }
>
> /**
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 9b058ee..9b0f568 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -219,7 +219,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
> {
> int retval = __srcu_read_lock(sp);
>
> - rcu_lock_acquire(&(sp)->dep_map);
> + __rcu_lock_acquire(&(sp)->dep_map, _THIS_IP_);
Good, we do not way srcu_read_lock() complaining about offline or idle
CPUs.
> return retval;
> }
>
> @@ -233,7 +233,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
> static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
> __releases(sp)
> {
> - rcu_lock_release(&(sp)->dep_map);
> + __rcu_lock_release(&(sp)->dep_map, _THIS_IP_);
> __srcu_read_unlock(sp, idx);
> }
>
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index a3596c8..19ff915 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -333,4 +333,47 @@ static int __init check_cpu_stall_init(void)
> }
> early_initcall(check_cpu_stall_init);
>
> +#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
> +
> +static void ck_rcu_is_watching(const char *message)
> +{
> + rcu_lockdep_assert(rcu_is_watching(), message);
> +}
> +
> +void rcu_lock_acquire(void)
> +{
> + __rcu_lock_acquire(&rcu_lock_map, _RET_IP_);
> + ck_rcu_is_watching("rcu_read_lock() used illegally while idle");
> +}
> +
> +void rcu_lock_release(void)
> +{
> + ck_rcu_is_watching("rcu_read_unlock() used illegally while idle");
> + __rcu_lock_release(&rcu_lock_map, _RET_IP_);
> +}
> +
> +void rcu_lock_acquire_bh(void)
> +{
> + __rcu_lock_acquire(&rcu_bh_lock_map, _RET_IP_);
> + ck_rcu_is_watching("rcu_read_lock_bh() used illegally while idle");
> +}
> +
> +void rcu_lock_release_bh(void)
> +{
> + ck_rcu_is_watching("rcu_read_unlock_bh() used illegally while idle");
> + __rcu_lock_release(&rcu_bh_lock_map, _RET_IP_);
> +}
> +void rcu_lock_acquire_sched(void)
> +{
> + __rcu_lock_acquire(&rcu_sched_lock_map, _RET_IP_);
> + ck_rcu_is_watching("rcu_read_lock_sched() used illegally while idle");
> +}
> +
> +void rcu_lock_release_sched(void)
> +{
> + ck_rcu_is_watching("rcu_read_unlock_sched() used illegally while idle");
> + __rcu_lock_release(&rcu_sched_lock_map, _RET_IP_);
> +}
> +#endif
> +
> #endif /* #ifdef CONFIG_RCU_STALL_COMMON */
>
next prev parent reply other threads:[~2014-01-22 3:54 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-09 11:15 [RFC][PATCH] lockdep: Introduce wait-type checks Peter Zijlstra
2014-01-09 11:49 ` Peter Zijlstra
2014-01-09 16:31 ` Oleg Nesterov
2014-01-09 17:08 ` Peter Zijlstra
2014-01-09 17:54 ` check && lockdep_no_validate (Was: lockdep: Introduce wait-type checks) Oleg Nesterov
2014-01-12 20:58 ` Peter Zijlstra
2014-01-13 16:07 ` Oleg Nesterov
2014-01-16 17:43 ` Oleg Nesterov
2014-01-16 18:09 ` Peter Zijlstra
2014-01-16 20:26 ` Alan Stern
2014-01-17 16:31 ` Oleg Nesterov
2014-01-17 18:01 ` Alan Stern
2014-01-20 18:19 ` [PATCH 0/5] lockdep: (Was: check && lockdep_no_validate) Oleg Nesterov
2014-01-20 18:20 ` [PATCH 1/5] lockdep: make held_lock->check and "int check" argument bool Oleg Nesterov
2014-02-10 13:32 ` [tip:core/locking] lockdep: Make " tip-bot for Oleg Nesterov
2014-01-20 18:20 ` [PATCH 2/5] lockdep: don't create the wrong dependency on hlock->check == 0 Oleg Nesterov
2014-02-10 13:33 ` [tip:core/locking] lockdep: Don' t " tip-bot for Oleg Nesterov
2014-01-20 18:20 ` [PATCH 3/5] lockdep: change mark_held_locks() to check hlock->check instead of lockdep_no_validate Oleg Nesterov
2014-02-10 13:33 ` [tip:core/locking] lockdep: Change " tip-bot for Oleg Nesterov
2014-01-20 18:20 ` [PATCH 4/5] lockdep: change lockdep_set_novalidate_class() to use _and_name Oleg Nesterov
2014-02-10 13:33 ` [tip:core/locking] lockdep: Change " tip-bot for Oleg Nesterov
2014-01-20 18:20 ` [PATCH 5/5] lockdep: pack subclass/trylock/read/check into a single argument Oleg Nesterov
2014-01-21 14:10 ` Peter Zijlstra
2014-01-21 17:27 ` Oleg Nesterov
2014-01-21 17:35 ` Dave Jones
2014-01-21 18:43 ` Oleg Nesterov
2014-01-21 18:53 ` Steven Rostedt
2014-01-21 20:06 ` Oleg Nesterov
2014-01-21 19:39 ` uninline rcu_lock_acquire/etc ? Oleg Nesterov
2014-01-22 3:54 ` Paul E. McKenney [this message]
2014-01-22 18:31 ` Oleg Nesterov
2014-01-22 19:34 ` Paul E. McKenney
2014-01-22 19:39 ` Oleg Nesterov
2014-01-20 18:37 ` [PATCH 0/5] lockdep: (Was: check && lockdep_no_validate) Alan Stern
2014-01-20 18:54 ` Oleg Nesterov
2014-01-20 21:42 ` Alan Stern
2014-01-12 9:40 ` [RFC][PATCH] lockdep: Introduce wait-type checks Ingo Molnar
2014-01-12 17:45 ` [PATCH 0/1] lockdep: Kill held_lock->check and "int check" arg of __lock_acquire() Oleg Nesterov
2014-01-12 17:45 ` [PATCH 1/1] " Oleg Nesterov
2014-01-13 0:28 ` Dave Jones
2014-01-13 16:20 ` Oleg Nesterov
2014-01-13 17:06 ` Oleg Nesterov
2014-01-13 17:28 ` Peter Zijlstra
2014-01-13 18:52 ` Oleg Nesterov
2014-01-13 22:34 ` Paul E. McKenney
2014-01-12 20:00 ` [PATCH 0/1] " Peter Zijlstra
2014-01-13 18:35 ` Oleg Nesterov
2014-01-09 17:33 ` [RFC][PATCH] lockdep: Introduce wait-type checks Dave Jones
2014-01-09 22:12 ` Peter Zijlstra
2014-01-10 12:11 ` 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=20140122035440.GW10038@linux.vnet.ibm.com \
--to=paulmck@linux.vnet.ibm.com \
--cc=davej@redhat.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=stern@rowland.harvard.edu \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.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).