Page MenuHomeFreeBSD
Differential D46842

LinuxKPI: update rcu_dereference_*() and lockdep_is_held()
ClosedPublic
Actions

Authored by bz on Sep 29 2024, 12:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Aug 18, 3:51 AM
Unknown Object (File)
Thu, Aug 13, 3:10 AM
Unknown Object (File)
Thu, Aug 13, 3:10 AM
Unknown Object (File)
Wed, Aug 12, 9:30 AM
Unknown Object (File)
Wed, Aug 12, 9:26 AM
Unknown Object (File)
Wed, Aug 12, 9:19 AM
Unknown Object (File)
Wed, Aug 12, 9:18 AM
Unknown Object (File)
Wed, Aug 12, 9:18 AM
View All Files
Subscribers
emaste
imp
kevans
wulf

Details

Reviewers
kevans
wulf
Group Reviewers
linuxkpi
Commits
rG83412843682b: LinuxKPI: update rcu_dereference_*() and lockdep_is_held()
rG5c92f84bb607: LinuxKPI: update rcu_dereference_*() and lockdep_is_held()
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.Sep 29 2024, 12:59 AM
Herald added subscribers: emaste, imp. · View Herald TranscriptSep 29 2024, 12:59 AM
bz requested review of this revision.Sep 29 2024, 12:59 AM
Harbormaster completed remote builds in B59649: Diff 143892.Sep 29 2024, 12:59 AM
bz planned changes to this revision.Oct 4 2024, 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.Oct 4 2024, 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.Oct 4 2024, 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().Oct 4 2024, 11:49 PM
bz edited the summary of this revision. (Show Details)
bz added a comment.Oct 10 2024, 11:24 PM

Anyone? x11 for drm-kmod testing at least?

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

return (true) is probably preferable

bz updated this revision to Diff 144663.Oct 11 2024, 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.Oct 11 2024, 5:50 PM
wulf added a subscriber: wulf.Oct 11 2024, 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.Oct 13 2024, 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.Oct 14 2024, 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.Oct 14 2024, 9:18 AM
  1. Linux name of this routine is (s)rcu_read_lock_held
bz updated this revision to Diff 144847.Oct 14 2024, 6:16 PM
bz marked 2 inline comments as done.

Update based on suggestions by @wulf

Harbormaster completed remote builds in B59985: Diff 144847.Oct 14 2024, 6:16 PM
wulf accepted this revision.Oct 16 2024, 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.Oct 16 2024, 12:13 PM
bz updated this revision to Diff 144989.Oct 16 2024, 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.Oct 16 2024, 2:26 PM
Harbormaster completed remote builds in B60038: Diff 144989.Oct 16 2024, 2:26 PM
bz added a comment.Oct 16 2024, 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.Oct 16 2024, 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.Oct 16 2024, 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.Oct 16 2024, 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.Oct 16 2024, 9:42 PM
wulf added a comment.Oct 17 2024, 11:00 AM

It works good on Intel Skylake

bz added a comment.Oct 17 2024, 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?

bz added a comment.Oct 21 2024, 7:32 AM
In D46842#1075364, @bz wrote:
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?

Unless I don't hear anything otherwise I'll commit this Tuesday 22. Oct.

wulf accepted this revision.Oct 22 2024, 11:23 AM

I am sorry for delay with review

This revision is now accepted and ready to land.Oct 22 2024, 11:23 AM
Closed by commit rG5c92f84bb607: LinuxKPI: update rcu_dereference_*() and lockdep_is_held() (authored by bz). · Explain WhyOct 23 2024, 4:17 AM
This revision was automatically updated to reflect the committed changes.
bz added a commit: rG5c92f84bb607: LinuxKPI: update rcu_dereference_*() and lockdep_is_held().
bz added a commit: rG83412843682b: LinuxKPI: update rcu_dereference_*() and lockdep_is_held().Oct 31 2024, 2:35 PM

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...