Reviewed by: Steve Gonczi <[email protected]>
Reviewed by: Pavel Zakharov <[email protected]>
Reviewed by: George Wilson <[email protected]>
Problem
Using the following stress test:
- Thread 1: In a loop, create and delete etherstub datalinks using:
dladm create-etherstub teststub0
dladm delete-etherstub teststub0
- Thread 2: In a loop, read the link kstat for teststub0.
- Thread 3: Do the same thing as Thread 2
After a while of running the above (~30 seconds), a deadlock ensues,
where Thread 1 is stuck unkillably in `dladm delete-etherstub`, and the
other threads are stuck in the kernel, in `read_kstat_data()` (called
from the `KSTAT_IOC_READ` ioctl). The two kernel threads stuck are:
ffffff03172f0b20 ffffff031d0bc028 ffffff036e341d00 1 59 ffffff038edd40c8
PC: _resume_from_idle+0xf4 CMD: dladm delete-etherstub -t teststub0
stack pointer for thread ffffff03172f0b20: ffffff000cd21810
[ ffffff000cd21810 _resume_from_idle+0xf4() ]
swtch+0x141()
cv_wait+0x70()
kstat_hold+0x4d()
kstat_hold_bykid+0x37()
kstat_delete+0x4e()
dls_devnet_stat_destroy+0x59()
dls_devnet_unset+0x1f3()
dls_devnet_destroy+0x46()
vnic_dev_delete+0x96()
vnic_ioc_delete+0x28()
drv_ioctl+0x1e4()
cdev_ioctl+0x39()
spec_ioctl+0x60()
fop_ioctl+0x55()
ioctl+0x9b()
_sys_sysenter_post_swapgs+0x149()
ffffff031733bb80 ffffff03188bb000 ffffff0315ad5380 1 59 ffffff0315b719d0
PC: _resume_from_idle+0xf4 CMD: /opt/jdk1.8.0_60/bin/java ...
stack pointer for thread ffffff031733bb80: ffffff000c3ea900
[ ffffff000c3ea900 _resume_from_idle+0xf4() ]
swtch+0x141()
turnstile_block+0x21a()
mutex_vector_enter+0x3a3()
dls_devnet_stat_update+0x3c()
read_kstat_data+0xd0()
kstat_ioctl+0x83()
cdev_ioctl+0x39()
spec_ioctl+0x60()
fop_ioctl+0x55()
ioctl+0x9b()
sys_syscall+0x17a()
Explanation
This is a classic lock ordering problem. The sequence of events between
Thread 1 and Thread 2 that causes this deadlock is:
1. Thread 1 calls dls_devnet_destroy() to delete the datalink. This
in turn causes this sequence of calls:
dls_devnet_unset()
mutex_enter(ddp->dd_mutex) <---- mutex aquired, function continues
...
2. Thread 2 issues the `KSTAT_IOC_READ` ioctl, enters
`read_kstat_data()`, which cause the following sequence of calls:
kstat_hold_by_ktid()
KSTAT_ENTER()
KSTAT_UPDATE()
dls_devnet_stat_update()
mutex_enter(ddp->dd_mutex) <---- blocks due to Thread 1
3. At this point, Thread 1 continues in `dls_devnet_unset()`:
dls_devnet_stat_destroy()
kstat_delete()
kstat_hold_by_ktid()
cv_wait() <----- waiting for Thread 2 to
signal that it has released
the kstat
At this point, Thread 1 and Thread 2 are deadlocked. Thread 1 is waiting
to be signaled by Thread 2, but Thread 2 is blocked waiting to acquire a
mutex that is held by Thread 1.
Solution
The fix is to always acquire a reference to a `dls_devnet_t` through the
same hash table lookup path so that lock ordering and destruction
semantics are always preserved. This means that instead of a back pointer
from `kstat_t` to `dls_devnet_t` (in the `ks_private` field), the
`ks_private` field will hold a `datalink_id_t` that will be used to call
`dls_devnet_hold_tmp()` to get at the `dls_devnet_t` in question.
This approach should have no performance impact, as the kstat update path
is not a datapath operation, the hash lookup is O(1), and the hash table
doesn't change frequently (only when datalinks are added or deleted).
tl;dr
In summary, the statistic subsystem must use the `dls_devnet_hold_tmp()`
API to obtain a temporary reference to a `dls_devnet_t`. This API is
designed to enforce proper locking and locking order, and safe access to
these objects so that threads don't race with deletions or renames.
This means that instead of having a direct back-pointer from a `kstat_t`
to a `dls_devnet_t` (`ks_private`), the `ks_private` field contains a
datalink ID which is used to lookup the `dls_devnet_t` using
`dls_devnet_hold_tmp()`.
Doing this allowed me to remove one ugly hack: Use of the
`DD_KSTAT_CHANGING` flag. I can remove this and all uses of it since
`dls_devnet_hold_tmp()` is designed to safely access datalinks that are
being renamed (unlike the old back-door access).
While I was in `dls_mgmt.c`, I noticed two `rw_enter()` calls that were
erroneously entering as writer instead of reader. I fixed those while I
was there.
Upstream bugs: DLPX-46888
You can view, comment on, or merge this pull request online at:
https://github.com/openzfs/openzfs/pull/365
-- Commit Summary --
* 8149 deadlock between datalink deletion and kstat read
-- File Changes --
M usr/src/pkg/manifests/system-test-ostest.mf (4)
M usr/src/test/os-tests/runfiles/default.run (4)
M usr/src/test/os-tests/tests/Makefile (4)
A usr/src/test/os-tests/tests/stress/Makefile (42)
A usr/src/test/os-tests/tests/stress/dladm-kstat.sh (67)
M usr/src/uts/common/io/dls/dls_mgmt.c (60)
-- Patch Links --
https://github.com/openzfs/openzfs/pull/365.patch
https://github.com/openzfs/openzfs/pull/365.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/365
------------------------------------------
openzfs-developer
Archives: https://openzfs.topicbox-beta.com/groups/developer/
Powered by Topicbox: https://topicbox-beta.com
