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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.