Page MenuHomeFreeBSD
Differential D46842

LinuxKPI: update rcu_dereference_*() and lockdep_is_held()
Needs ReviewPublic
Actions

Authored by bz on Sun, Sep 29, 12:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 17, 9:53 AM
Unknown Object (File)
Wed, Oct 16, 3:14 PM
Unknown Object (File)
Mon, Oct 7, 6:34 PM
Unknown Object (File)
Mon, Oct 7, 10:51 AM
Unknown Object (File)
Fri, Oct 4, 1:26 PM
Unknown Object (File)
Fri, Oct 4, 10:35 AM
Unknown Object (File)
Tue, Oct 1, 11:53 AM
Subscribers
emaste
imp
kevans
wulf

Details

Reviewers
kevans
wulf
Group Reviewers
linuxkpi
Summary

Update rcu_dereference_{check,protected}() to call the check and log
once if if fails and if the RCU debug sysctl is turned on.
Also add proper checks for conditions passed in to these functions.
For that implement linux_rcu_read_lock_locked().

(While here also remove extraneous extern for function prototypes).

Update lockdep_is_held() to always be an inline function with argument
annotation so that we do no longer have unused variables
in callers which only call lockdep_is_held().

Sponsored by: The FreeBSD Foundation
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 59911
Build 56796: arc lint + arc unit

Event Timeline

bz created this revision.Sun, Sep 29, 12:59 AM
Herald added subscribers: emaste, imp. · View Herald TranscriptSun, Sep 29, 12:59 AM
bz requested review of this revision.Sun, Sep 29, 12:59 AM
Harbormaster completed remote builds in B59649: Diff 143892.Sun, Sep 29, 12:59 AM
bz planned changes to this revision.Fri, Oct 4, 8:51 PM

These give false positives given check is aliased to it as well. I am working on an update.

bz updated this revision to Diff 144292.Fri, Oct 4, 11:48 PM

Updated with proper condition checks also for the "check" variant and
added a sysctl to enable the RCU debug logging as it seems it could
possibly be noisy currently.

Harbormaster completed remote builds in B59780: Diff 144292.Fri, Oct 4, 11:48 PM
bz retitled this revision from LinuxKPI: update rcu_dereference_protected() and lockdep_is_held() to LinuxKPI: update rcu_dereference_*() and lockdep_is_held().Fri, Oct 4, 11:49 PM
bz edited the summary of this revision. (Show Details)
bz added a comment.Thu, Oct 10, 11:24 PM

Anyone? x11 for drm-kmod testing at least?

emaste added inline comments.Fri, Oct 11, 2:28 PM
sys/compat/linuxkpi/common/include/linux/lockdep.h
95

return (true) is probably preferable

bz updated this revision to Diff 144663.Fri, Oct 11, 5:50 PM

Return true instea dof the 1 lockdep_is_held used to return as suggested by @emaste

Harbormaster completed remote builds in B59911: Diff 144663.Fri, Oct 11, 5:50 PM
wulf added a subscriber: wulf.Fri, Oct 11, 8:28 PM
wulf added inline comments.
sys/compat/linuxkpi/common/src/linux_rcu.c
270

Our in_epoch() does different thing in preemptible case. See sys/kern/subr_epoch.c:in_epoch_verbose_preempt()
It scans through list of threads entered in to the epoch section and compares each entry with curthread.
Probably we should do the same with only difference of comparing each entry with current.

But I am not an RCU/epoch expert.

bz added a reviewer: kevans.Sun, Oct 13, 10:58 PM
bz marked an inline comment as done.
bz added a subscriber: kevans.
bz added inline comments.
sys/compat/linuxkpi/common/src/linux_rcu.c
263

Insert:

/* If the thread is not pinned it is not in an epoch. */
if (td->td_pinned == 0)
        return (false);
270

Adding @kevans given he wrote that code in sys/kern/subr_epoch.c

The more I look at the surrounding code tonight the more I am confused about the possibility of a race condition.

wulf added inline comments.Mon, Oct 14, 9:09 AM
sys/compat/linuxkpi/common/src/linux_rcu.c
270

At second glance:

  1. current should be checked to be initialized before usage as it may results in unexpected malloc()
  2. ts->rcu_recurse[type] check is enough
  3. td->td_pinned and epoch_record.active checks are not needed but may be MPASS-ed in positive case
  4. No need in critical section as it protects atomic operation (aligned int read)
wulf added a comment.Mon, Oct 14, 9:18 AM
  1. Linux name of this routine is (s)rcu_read_lock_held
bz updated this revision to Diff 144847.Mon, Oct 14, 6:16 PM
bz marked 2 inline comments as done.

Update based on suggestions by @wulf

Harbormaster completed remote builds in B59985: Diff 144847.Mon, Oct 14, 6:16 PM
wulf accepted this revision.Wed, Oct 16, 12:13 PM

The only note: It looks that rcu_read_lock_held() always returns true on non-debug builds: https://elixir.bootlin.com/linux/v6.11.3/source/include/linux/rcupdate.h#L351

This revision is now accepted and ready to land.Wed, Oct 16, 12:13 PM
bz updated this revision to Diff 144989.Wed, Oct 16, 2:26 PM

Wrap the rcu_read_lock_held() implementation in INVARIANTS and otherwise
just return true as pointed out by @wulf.

This revision now requires review to proceed.Wed, Oct 16, 2:26 PM
Harbormaster completed remote builds in B60038: Diff 144989.Wed, Oct 16, 2:26 PM
bz added a comment.Wed, Oct 16, 2:26 PM
In D46842#1074849, @wulf wrote:

The only note: It looks that rcu_read_lock_held() always returns true on non-debug builds: https://elixir.bootlin.com/linux/v6.11.3/source/include/linux/rcupdate.h#L351

Done. If you are happy, did you by any chance test this with some drm-kmod graphics that we don't run into surprises there?

wulf added a comment.Wed, Oct 16, 3:31 PM

Heh, this broke build of drm-kmod's master branch:

/home/wulf/dvp/drm-kmod/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c:187:15: error: expression result unused [-Werror,-Wunused-value]
  187 |         if (unlikely(rcu_dereference_protected(*ptr, 1))) {
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/rcupdate.h:115:5: note: expanded from macro 'rcu_dereference_protected'
  115 |     __rcu_dereference_protected((p), (c),                               \
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  116 |         __rcu_var_name(protected, __func__, __LINE__))
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/rcupdate.h:109:5: note: expanded from macro '__rcu_dereference_protected'
  109 |     RCU_WARN_ONCE(!(c), "%s:%d: condition for %s failed\n",             \
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  110 |         __func__, __LINE__, __XSTRING(n));                              \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/rcupdate.h:45:3: note: expanded from macro 'RCU_WARN_ONCE'
   45 |                 1;                                                      \
      |                 ^
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/compiler.h:82:43: note: expanded from macro 'unlikely'
   82 | #define unlikely(x)                     __builtin_expect(!!(x), 0)
bz added a comment.Wed, Oct 16, 9:40 PM
In D46842#1074971, @wulf wrote:

Heh, this broke build of drm-kmod's master branch:

/home/wulf/dvp/drm-kmod/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c:187:15: error: expression result unused [-Werror,-Wunused-value]
  187 |         if (unlikely(rcu_dereference_protected(*ptr, 1))) {
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/rcupdate.h:115:5: note: expanded from macro 'rcu_dereference_protected'
  115 |     __rcu_dereference_protected((p), (c),                               \
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  116 |         __rcu_var_name(protected, __func__, __LINE__))
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/rcupdate.h:109:5: note: expanded from macro '__rcu_dereference_protected'
  109 |     RCU_WARN_ONCE(!(c), "%s:%d: condition for %s failed\n",             \
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  110 |         __func__, __LINE__, __XSTRING(n));                              \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/rcupdate.h:45:3: note: expanded from macro 'RCU_WARN_ONCE'
   45 |                 1;                                                      \
      |                 ^
/home/wulf/dvp/freebsd/wulf/sys/compat/linuxkpi/common/include/linux/compiler.h:82:43: note: expanded from macro 'unlikely'
   82 | #define unlikely(x)                     __builtin_expect(!!(x), 0)

I have an update (also compiling 5.1-lts) but drm-kmod master is still broken for LINT kernels in ACPI.
Will upload updated diff in a few minutes.

bz updated this revision to Diff 145011.Wed, Oct 16, 9:42 PM

Update to unbreak drm-kmod (master/LINT still fails in ACPI, GENERIC* compiled; 6.1-lts also compiled fully)

Harbormaster completed remote builds in B60051: Diff 145011.Wed, Oct 16, 9:42 PM
wulf added a comment.Thu, Oct 17, 11:00 AM

It works good on Intel Skylake

bz added a comment.Thu, Oct 17, 5:59 PM
In D46842#1075167, @wulf wrote:

It works good on Intel Skylake

I also only have intel to test with; does that mean this is good to go?

Revision Contents
Changeset List

PathSize
sys/
compat/
linuxkpi/
common/
include/
linux/
28 lines
60 lines
src/
4 lines
28 lines

Diff 144663

View Options

sys/compat/linuxkpi/common/include/linux/lockdep.h

Loading...
View Options

sys/compat/linuxkpi/common/include/linux/rcupdate.h

Loading...
View Options

sys/compat/linuxkpi/common/src/linux_compat.c

Loading...
View Options

sys/compat/linuxkpi/common/src/linux_rcu.c

Loading...