list_for_each_entry_rcu now has support to check for RCU reader sections
as well as lock. Just use the support in it, instead of explicitly
checking in the caller.
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
kernel/workqueue.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 601d61150b65..e882477ebf6e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
!lockdep_is_held(&wq_pool_mutex), \
"RCU or wq_pool_mutex should be held")
-#define assert_rcu_or_wq_mutex(wq) \
- RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
- !lockdep_is_held(&wq->mutex), \
- "RCU or wq->mutex should be held")
-
#define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
!lockdep_is_held(&wq->mutex) && \
@@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
* ignored.
*/
#define for_each_pwq(pwq, wq) \
- list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
- if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
- else
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
+ lock_is_held(&(wq->mutex).dep_map))
#ifdef CONFIG_DEBUG_OBJECTS_WORK
--
2.23.0.rc1.153.gdeed80330f-goog
This patch updates the documentation with information about
usage of lockdep with list_for_each_entry_rcu().
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
Documentation/RCU/lockdep.txt | 15 +++++++++++----
Documentation/RCU/whatisRCU.txt | 9 ++++++++-
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/Documentation/RCU/lockdep.txt b/Documentation/RCU/lockdep.txt
index da51d3068850..3d967df3a801 100644
--- a/Documentation/RCU/lockdep.txt
+++ b/Documentation/RCU/lockdep.txt
@@ -96,7 +96,14 @@ other flavors of rcu_dereference(). On the other hand, it is illegal
to use rcu_dereference_protected() if either the RCU-protected pointer
or the RCU-protected data that it points to can change concurrently.
-There are currently only "universal" versions of the rcu_assign_pointer()
-and RCU list-/tree-traversal primitives, which do not (yet) check for
-being in an RCU read-side critical section. In the future, separate
-versions of these primitives might be created.
+Similar to rcu_dereference_protected, The RCU list and hlist traversal
+primitives also check for whether there are called from within a reader
+section. However, an optional lockdep expression can be passed to them as
+the last argument in case they are called under other non-RCU protection.
+
+For example, the workqueue for_each_pwq() macro is implemented as follows.
+It is safe to call for_each_pwq() outside a reader section but under protection
+of wq->mutex:
+#define for_each_pwq(pwq, wq)
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node,
+ lock_is_held(&(wq->mutex).dep_map))
diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index 17f48319ee16..cdd2a3e10e40 100644
--- a/Documentation/RCU/whatisRCU.txt
+++ b/Documentation/RCU/whatisRCU.txt
@@ -290,7 +290,7 @@ rcu_dereference()
at any time, including immediately after the rcu_dereference().
And, again like rcu_assign_pointer(), rcu_dereference() is
typically used indirectly, via the _rcu list-manipulation
- primitives, such as list_for_each_entry_rcu().
+ primitives, such as list_for_each_entry_rcu() [2].
[1] The variant rcu_dereference_protected() can be used outside
of an RCU read-side critical section as long as the usage is
@@ -305,6 +305,13 @@ rcu_dereference()
a lockdep splat is emitted. See Documentation/RCU/Design/Requirements/Requirements.rst
and the API's code comments for more details and example usage.
+ [2] In case the list_for_each_entry_rcu() primitive is intended
+ to be used outside of an RCU reader section such as when
+ protected by a lock, then an additional lockdep expression can be
+ passed as the last argument to it so that RCU lockdep checking code
+ knows that the dereference of the list pointers are safe. If the
+ indicated protection is not provided, a lockdep splat is emitted.
+
The following diagram shows how each API communicates among the
reader, updater, and reclaimer.
--
2.23.0.rc1.153.gdeed80330f-goog
On Sun, Aug 11, 2019 at 6:11 PM Joel Fernandes (Google)
<[email protected]> wrote:
>
> list_for_each_entry_rcu now has support to check for RCU reader sections
> as well as lock. Just use the support in it, instead of explicitly
> checking in the caller.
>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
Tejun,
Could you please Ack this patch? I have resent it here.
Thank you,
- Joel
> ---
> kernel/workqueue.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 601d61150b65..e882477ebf6e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> !lockdep_is_held(&wq_pool_mutex), \
> "RCU or wq_pool_mutex should be held")
>
> -#define assert_rcu_or_wq_mutex(wq) \
> - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> - !lockdep_is_held(&wq->mutex), \
> - "RCU or wq->mutex should be held")
> -
> #define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
> RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> !lockdep_is_held(&wq->mutex) && \
> @@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> * ignored.
> */
> #define for_each_pwq(pwq, wq) \
> - list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
> - if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
> - else
> + list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
> + lock_is_held(&(wq->mutex).dep_map))
>
> #ifdef CONFIG_DEBUG_OBJECTS_WORK
>
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
Properly check if lockdep lock checking is disabled at config time. If
so, then lock_is_held() is undefined so don't do any checking.
This fix is similar to the pattern used in srcu_read_lock_held().
Link: https://lore.kernel.org/lkml/201908080026.WSAFx14k%[email protected]/
Fixes: c9e4d3a2fee8 ("acpi: Use built-in RCU list checking for acpi_ioremaps list")
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
This patch is based on the -rcu dev branch.
drivers/base/core.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 32cf83d1c744..fe25cf690562 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -99,7 +99,11 @@ void device_links_read_unlock(int not_used)
int device_links_read_lock_held(void)
{
- return lock_is_held(&device_links_lock);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ return lock_is_held(&(device_links_lock.dep_map));
+#else
+ return 1;
+#endif
}
#endif /* !CONFIG_SRCU */
--
2.23.0.rc1.153.gdeed80330f-goog
On Sun, Aug 11, 2019 at 06:11:11PM -0400, Joel Fernandes (Google) wrote:
> Properly check if lockdep lock checking is disabled at config time. If
> so, then lock_is_held() is undefined so don't do any checking.
>
> This fix is similar to the pattern used in srcu_read_lock_held().
>
> Link: https://lore.kernel.org/lkml/201908080026.WSAFx14k%[email protected]/
> Fixes: c9e4d3a2fee8 ("acpi: Use built-in RCU list checking for acpi_ioremaps list")
What tree is this commit in?
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> ---
> This patch is based on the -rcu dev branch.
Ah...
> drivers/base/core.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 32cf83d1c744..fe25cf690562 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -99,7 +99,11 @@ void device_links_read_unlock(int not_used)
>
> int device_links_read_lock_held(void)
> {
> - return lock_is_held(&device_links_lock);
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> + return lock_is_held(&(device_links_lock.dep_map));
> +#else
> + return 1;
> +#endif
return 1? So the lock is always held?
confused,
greg k-h
On Mon, Aug 12, 2019 at 07:02:56AM +0200, Greg Kroah-Hartman wrote:
> On Sun, Aug 11, 2019 at 06:11:11PM -0400, Joel Fernandes (Google) wrote:
> > Properly check if lockdep lock checking is disabled at config time. If
> > so, then lock_is_held() is undefined so don't do any checking.
> >
> > This fix is similar to the pattern used in srcu_read_lock_held().
> >
> > Link: https://lore.kernel.org/lkml/201908080026.WSAFx14k%[email protected]/
> > Fixes: c9e4d3a2fee8 ("acpi: Use built-in RCU list checking for acpi_ioremaps list")
>
> What tree is this commit in?
>
> > Reported-by: kbuild test robot <[email protected]>
> > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> > ---
> > This patch is based on the -rcu dev branch.
>
> Ah...
>
> > drivers/base/core.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 32cf83d1c744..fe25cf690562 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -99,7 +99,11 @@ void device_links_read_unlock(int not_used)
> >
> > int device_links_read_lock_held(void)
> > {
> > - return lock_is_held(&device_links_lock);
> > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > + return lock_is_held(&(device_links_lock.dep_map));
> > +#else
> > + return 1;
> > +#endif
>
> return 1? So the lock is always held?
This is just the pattern of an assert that is disabled, so that
false-positives don't happen if lockdep is disabled.
So say someone writes a statement like:
WARN_ON_ONCE(!device_links_read_lock_held());
Since lockdep is disabled, we cannot check whether lock is held or not. Yet,
we don't want false positives by reporting that the lock is not held. In this
case, it is better to report that the lock is held to suppress
false-positives. srcu_read_lock_held() also follows the same pattern.
thanks,
- Joel
On Mon, 12 Aug 2019 09:03:10 -0400
Joel Fernandes <[email protected]> wrote:
> > > drivers/base/core.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 32cf83d1c744..fe25cf690562 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -99,7 +99,11 @@ void device_links_read_unlock(int not_used)
> > >
> > > int device_links_read_lock_held(void)
> > > {
> > > - return lock_is_held(&device_links_lock);
> > > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > + return lock_is_held(&(device_links_lock.dep_map));
> > > +#else
> > > + return 1;
> > > +#endif
> >
> > return 1? So the lock is always held?
I was thinking the exact same thing.
>
> This is just the pattern of an assert that is disabled, so that
> false-positives don't happen if lockdep is disabled.
>
> So say someone writes a statement like:
> WARN_ON_ONCE(!device_links_read_lock_held());
>
> Since lockdep is disabled, we cannot check whether lock is held or not. Yet,
> we don't want false positives by reporting that the lock is not held. In this
> case, it is better to report that the lock is held to suppress
> false-positives. srcu_read_lock_held() also follows the same pattern.
>
The real answer here is to make that WARN_ON_ONCE() dependent on
lockdep. Something like:
some/header/file.h:
#ifdef CONFIG_DEBUG_LOCK_ALLOC
# define CHECK_DEVICE_LINKS_READ_LOCK_HELD() WARN_ON_ONCE(!defice_links_read_lock_held())
#else
# define CHECK_DEVICE_LINKS_READ_LOCK_HELD() do { } while (0)
#endif
And just use CHECK_DEVICE_LINK_READ_LOCK_HELD() in those places. I
agree with Greg. "device_links_read_lock_heald()" should *never*
blindly return 1. It's confusing.
-- Steve
On Mon, Aug 12, 2019 at 02:11:19PM -0400, Steven Rostedt wrote:
> On Mon, 12 Aug 2019 09:03:10 -0400
> Joel Fernandes <[email protected]> wrote:
>
>
> > > > drivers/base/core.c | 6 +++++-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 32cf83d1c744..fe25cf690562 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -99,7 +99,11 @@ void device_links_read_unlock(int not_used)
> > > >
> > > > int device_links_read_lock_held(void)
> > > > {
> > > > - return lock_is_held(&device_links_lock);
> > > > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > > + return lock_is_held(&(device_links_lock.dep_map));
> > > > +#else
> > > > + return 1;
> > > > +#endif
> > >
> > > return 1? So the lock is always held?
>
> I was thinking the exact same thing.
>
> >
> > This is just the pattern of an assert that is disabled, so that
> > false-positives don't happen if lockdep is disabled.
> >
> > So say someone writes a statement like:
> > WARN_ON_ONCE(!device_links_read_lock_held());
> >
> > Since lockdep is disabled, we cannot check whether lock is held or not. Yet,
> > we don't want false positives by reporting that the lock is not held. In this
> > case, it is better to report that the lock is held to suppress
> > false-positives. srcu_read_lock_held() also follows the same pattern.
> >
>
> The real answer here is to make that WARN_ON_ONCE() dependent on
> lockdep. Something like:
>
>
> some/header/file.h:
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> # define CHECK_DEVICE_LINKS_READ_LOCK_HELD() WARN_ON_ONCE(!defice_links_read_lock_held())
> #else
> # define CHECK_DEVICE_LINKS_READ_LOCK_HELD() do { } while (0)
> #endif
>
> And just use CHECK_DEVICE_LINK_READ_LOCK_HELD() in those places. I
> agree with Greg. "device_links_read_lock_heald()" should *never*
> blindly return 1. It's confusing.
Ok, then I will update the patch to do:
#ifdef CONFIG_DEBUG_LOCK_ALLOC
int device_links_read_lock_held(void)
{
return lock_is_held(&device_links_lock);
}
#endif
That will also solve the build error. And callers can follow the above pattern you shared.
thanks,
- Joel
On Mon, 12 Aug 2019 16:01:25 -0400
Joel Fernandes <[email protected]> wrote:
> > some/header/file.h:
> >
> > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > # define CHECK_DEVICE_LINKS_READ_LOCK_HELD() WARN_ON_ONCE(!defice_links_read_lock_held())
> > #else
> > # define CHECK_DEVICE_LINKS_READ_LOCK_HELD() do { } while (0)
> > #endif
> >
> > And just use CHECK_DEVICE_LINK_READ_LOCK_HELD() in those places. I
> > agree with Greg. "device_links_read_lock_heald()" should *never*
> > blindly return 1. It's confusing.
>
> Ok, then I will update the patch to do:
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> int device_links_read_lock_held(void)
> {
> return lock_is_held(&device_links_lock);
> }
> #endif
>
> That will also solve the build error. And callers can follow the above pattern you shared.
Sounds good!
-- Steve
On Mon, Aug 12, 2019 at 01:22:41PM -0700, Paul E. McKenney wrote:
> On Sun, Aug 11, 2019 at 06:11:10PM -0400, Joel Fernandes (Google) wrote:
> > This patch updates the documentation with information about
> > usage of lockdep with list_for_each_entry_rcu().
> >
> > Signed-off-by: Joel Fernandes (Google) <[email protected]>
>
> Thank you!!!
>
> I queued this for v5.5 with the following wordsmithing. Please check
> to make sure that I didn't mess anything up.
Yes, this looks great to me. thanks!
Also, I will send out the other drivers/core patch of this series in a bit
with Steve's suggestion.
- Joel
[snip]
On Sun, Aug 11, 2019 at 06:11:10PM -0400, Joel Fernandes (Google) wrote:
> This patch updates the documentation with information about
> usage of lockdep with list_for_each_entry_rcu().
>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
Thank you!!!
I queued this for v5.5 with the following wordsmithing. Please check
to make sure that I didn't mess anything up.
Thanx, Paul
------------------------------------------------------------------------
commit d06933df6b5919abfd298291f2a6b0a3a095ae64
Author: Joel Fernandes (Google) <[email protected]>
Date: Sun Aug 11 18:11:10 2019 -0400
doc: Update list_for_each_entry_rcu() documentation
This commit updates the documentation with information about
usage of lockdep with list_for_each_entry_rcu().
Signed-off-by: Joel Fernandes (Google) <[email protected]>
[ paulmck: Wordsmithing. ]
Signed-off-by: Paul E. McKenney <[email protected]>
diff --git a/Documentation/RCU/lockdep.txt b/Documentation/RCU/lockdep.txt
index da51d3068850..89db949eeca0 100644
--- a/Documentation/RCU/lockdep.txt
+++ b/Documentation/RCU/lockdep.txt
@@ -96,7 +96,17 @@ other flavors of rcu_dereference(). On the other hand, it is illegal
to use rcu_dereference_protected() if either the RCU-protected pointer
or the RCU-protected data that it points to can change concurrently.
-There are currently only "universal" versions of the rcu_assign_pointer()
-and RCU list-/tree-traversal primitives, which do not (yet) check for
-being in an RCU read-side critical section. In the future, separate
-versions of these primitives might be created.
+Like rcu_dereference(), when lockdep is enabled, RCU list and hlist
+traversal primitives check for being called from within an RCU read-side
+critical section. However, a lockdep expression can be passed to them
+as a additional optional argument. With this lockdep expression, these
+traversal primitives will complain only if the lockdep expression is
+false and they are called from outside any RCU read-side critical section.
+
+For example, the workqueue for_each_pwq() macro is intended to be used
+either within an RCU read-side critical section or with wq->mutex held.
+It is thus implemented as follows:
+
+ #define for_each_pwq(pwq, wq)
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node,
+ lock_is_held(&(wq->mutex).dep_map))
diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index 17f48319ee16..58ba05c4d97f 100644
--- a/Documentation/RCU/whatisRCU.txt
+++ b/Documentation/RCU/whatisRCU.txt
@@ -290,7 +290,7 @@ rcu_dereference()
at any time, including immediately after the rcu_dereference().
And, again like rcu_assign_pointer(), rcu_dereference() is
typically used indirectly, via the _rcu list-manipulation
- primitives, such as list_for_each_entry_rcu().
+ primitives, such as list_for_each_entry_rcu() [2].
[1] The variant rcu_dereference_protected() can be used outside
of an RCU read-side critical section as long as the usage is
@@ -305,6 +305,14 @@ rcu_dereference()
a lockdep splat is emitted. See Documentation/RCU/Design/Requirements/Requirements.rst
and the API's code comments for more details and example usage.
+ [2] If the list_for_each_entry_rcu() instance might be used by
+ update-side code as well as by RCU readers, then an additional
+ lockdep expression can be added to its list of arguments.
+ For example, given an additional "lock_is_held(&mylock)" argument,
+ the RCU lockdep code would complain only if this instance was
+ invoked outside of an RCU read-side critical section and without
+ the protection of mylock.
+
The following diagram shows how each API communicates among the
reader, updater, and reclaimer.
On Mon, Aug 12, 2019 at 04:42:05PM -0400, Joel Fernandes wrote:
> On Mon, Aug 12, 2019 at 01:22:41PM -0700, Paul E. McKenney wrote:
> > On Sun, Aug 11, 2019 at 06:11:10PM -0400, Joel Fernandes (Google) wrote:
> > > This patch updates the documentation with information about
> > > usage of lockdep with list_for_each_entry_rcu().
> > >
> > > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> >
> > Thank you!!!
> >
> > I queued this for v5.5 with the following wordsmithing. Please check
> > to make sure that I didn't mess anything up.
>
> Yes, this looks great to me. thanks!
>
> Also, I will send out the other drivers/core patch of this series in a bit
> with Steve's suggestion.
Very good! I do need acks from maintainers before sending these upstream,
though.
Thanx, Paul
Hello, Joel.
On Sun, Aug 11, 2019 at 06:11:09PM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu now has support to check for RCU reader sections
> as well as lock. Just use the support in it, instead of explicitly
> checking in the caller.
>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
Acked-by: Tejun Heo <[email protected]>
> #define for_each_pwq(pwq, wq) \
> - list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
> - if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
> - else
> + list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
> + lock_is_held(&(wq->mutex).dep_map))
Why not lockdep_is_held() tho?
Thanks.
--
tejun
On Wed, Aug 14, 2019 at 12:48:41PM -0700, Tejun Heo wrote:
> Hello, Joel.
>
> On Sun, Aug 11, 2019 at 06:11:09PM -0400, Joel Fernandes (Google) wrote:
> > list_for_each_entry_rcu now has support to check for RCU reader sections
> > as well as lock. Just use the support in it, instead of explicitly
> > checking in the caller.
> >
> > Signed-off-by: Joel Fernandes (Google) <[email protected]>
>
> Acked-by: Tejun Heo <[email protected]>
Thanks.
> > #define for_each_pwq(pwq, wq) \
> > - list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
> > - if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
> > - else
> > + list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
> > + lock_is_held(&(wq->mutex).dep_map))
>
> Why not lockdep_is_held() tho?
Yes, that's better.
thanks,
- Joel