Taylor R Campbell
2022-01-09 02:03:24 UTC
The attached patch set fixes a deadlock in usbnet and considerably
simplifies the API. This changes both the API and ABI so it'll
require a kernel bump. Attached is an incremental patch set to make
review and bisection easier, and a giant diff for testing.
Review and testing welcome!
I have successfully tested aue(4), ure(4), and url(4), with dhcpcd,
mdnsd, ifconfig down up in a loop, and detaching/reattaching
physically, all at the same time. (I also tested ping and data
transfer too! But mostly this patch set affects the init/stop paths,
not the tx/rx paths.) So far I haven't won the game of `crash or
deadlock the system' this way!
It wouldn't hurt to test more of those devices, of course, but I also
don't have (working) devices handy for the following:
axe(4), axen(4), cdce(4), cue(4), kue(4), mos(4), mue(4), smsc(4),
udav(4), upl(4), urndis(4)
DEADLOCK. The deadlock happens when a device such as ure(4) is
unplugged:
1. usbnet_detach holds the usbnet core lock, awaits softclock `lock'
(i.e., softints running sequentially at each level on a single CPU)
for callout to wake kpause in ure_reset
2. callout holds softclock `lock', awaits softnet_lock in
tcp_timer_keep, frag6_fasttimo, &c.
3. soclose holds softnet_lock, awaits usbnet core lock in ure_ioctl
for SIOCDELMULTI
This patch set breaks the deadlock by handling hardware multicast
filter updates for SIOCADDMULTI/SIOCDELMULTI through a new usbnet
driver operation, uno_mcast, rather than through uno_ioctl, which
doesn't need the usbnet core lock or IFNET_LOCK.
The job of uno_mcast is to commit the ethersubr(9) multicast address
list to any hardware filter. usbnet(9) serializes calls to uno_mcast,
and only while the interface is up. After uno_stop, SIOCADDMULTI and
SIOCDELMULTI will still update the ethersubr(9) list, but usbnet(9)
won't bother calling uno_mcast until the next if_init, at which time
it will atomically call uno_mcast and begin accepting concurrent
multicast updates.
Note: uno_mcast is issued synchronously. However, the patch set makes
it asynchronous temporarily, so we can pull it up to netbsd-9, because
a delayed multicast filter update is better than a deadlock; later in
the patch set the synchronous updates are restored with a different
locking ABI.
(Could the deadlock have been done more simply, by teaching the
various *_uno_ioctl routines to not take the usbnet core lock for
SIOCADDMULTI/SIOCDELMULTI? They could do that -- but then they race
with init/stop, which is a whole nother can of worms. This patch set
addresses that in a much more regimented flow of driver callbacks.)
API CHANGES. There is now a rigid timeline on which usbnet(9) issues
driver callbacks (time flows downward; comma means `may happen in
parallel'; => means `invoked synchronously as a subroutine'):
usbnet_attach
(driver has exclusive access to everything here, no locking needed)
usbnet_attach_ifp
uno_ioctl, uno_read_reg or uno_write_reg or uno_statchg
ifp->if_init
=> uno_init
=> uno_mcast
uno_tx_prepare, uno_rx_loop, uno_tick, uno_intr, uno_ioctl, uno_mcast,
uno_read_reg or uno_write_reg or uno_statchg
ifp->if_stop
=> uno_stop
uno_ioctl, uno_read_reg or uno_write_reg or uno_statchg
usbnet_detach
Notes:
- Repeated if_init while IFF_RUNNING will be ignored, so uno_init is
only called on the interface-up transition from !IFF_RUNNING to
IFF_RUNNING, and not again until after stopping.
- Similarly, repeated if_stop while !IFF_RUNNING will be ignored, so
uno_stop is only called on the interface-down transition from
IFF_RUNNING to !IFF_RUNNING (and then only when the device is not
detaching -- there's no point in trying to write device registers to
reset the hardware if it's physically gone!), and not again until
after reinitialization.
- uno_init, uno_stop, and uno_ioctl are always called with IFNET_LOCK
held, the long-term interface configuration lock.
- Each of
. uno_tx_prepare
. uno_rx_loop
. uno_tick
. uno_intr
. uno_ioctl
. uno_mcast
. uno_read_reg or uno_write_reg or uno_statchg
independently will only ever be called in serial -- that is, e.g.,
there will never be two concurrent calls to uno_mcast for the same
usbnet device, or two concurrent calls to uno_rx_loop. But there
may be concurrent calls to uno_rx_loop and uno_mcast -- one of each.
- The MII callbacks uno_read_reg, uno_write_reg, and uno_statchg are
serialized among themselves -- there is never an uno_read_reg and
uno_write_reg at the same time on the same usbnet device, for
instance. However, they may run in parallel with anything else,
except uno_init or uno_stop, at any time between usbnet_attach_ifp
and usbnet_detach, irrespective of whether the interface is up or
down.
API DELETIONS. The following usbnet API functions are now gone:
- usbnet_set_dying -- only one driver, url(4), ever used this, and
only on attach failure, which usbnet already gracefully handles
- usbnet_lock_{core,tx,rx} &c. -- these locks are now internal to
usbnet.c; drivers have no need to take them
- usbnet_busy, usbnet_unbusy -- nothing relied on usbnet reference
counting; usbnet waits for the users of all resources to drain by
some other mechanism -- callout_halt, usb_rem_task_wait, if_detach,
&c. -- before destroying them, following the rigid timeline.
- usbnet_init_rx_tx -- now handled internally by usbnet's if_init
function; this only contained driver-independent logic to set up
pipes and callouts and, after changes for the hardware multicast
filter updates, was used the same way by all drivers as the last
thing done by uno_init.
- usbnet_stop -- now handled internally by usbnet's if_stop function;
contained driver-independent logic to tear down pipes and callouts,
along with a call to uno_stop. The driver-independent logic now
rigidly follows the timeline above; there is no need for drivers to
invoke it directly.
BUG IN MUE(4). Currently it is possible for uno_ioctl and uno_mcast to
run concurrently. This means there is a bug in mue(4), because they
touch the same registers. Two alternatives are:
1. Make uno_ioctl take the same lock inside usbnet as uno_mcast.
=> I'm reluctant because I don't know if some ioctls might
reasonably be blocking, which is not allowed for
SIOCADDMULTI/SIOCDELMULTI to wait for. But currently I don't
think this poses a problem with the usbnet drivers in tree --
I'm just worried about deadlocks introduced by future drivers
with fancier ioctls.
2. Create a new lock inside mue(4) for mue_uno_cast and
mue_sethwcsum_locked.
=> Makes things a little more complicated in the driver -- but the
complication of additional interactions is limited to this
driver, so maybe not so bad.
Preferences?
BUG IN SMSC(4). Currently there is a minor bug in smsc(4) because
smsc_setoe_locked, which services SIOCSIFCAP to enable or disable
hardware checksumming, races with smsc_uno_rx_loop over
sc->sc_coe_ctrl. Here are two way we might address this:
1. Create a lock that smsc_setoe_locked and smsc_uno_rx_loop agree on
to cover sc->sc_coe_ctrl or manage it with atomics, and if the rx
already happened (because the hardware packet delivery isn't
beholden to any kmutex_t we create), just accept that we might lose
some packets to checksum errors.
2. Make SIOCSIFCAP take the interface down, set the checksum cap, and
then bring the interface back up again.
I'm inclined to prefer option (1); option (2) means we'll still drop
some packets -- probably more! -- and it's more intrusive into the
flow of logic.
Preferences?
simplifies the API. This changes both the API and ABI so it'll
require a kernel bump. Attached is an incremental patch set to make
review and bisection easier, and a giant diff for testing.
Review and testing welcome!
I have successfully tested aue(4), ure(4), and url(4), with dhcpcd,
mdnsd, ifconfig down up in a loop, and detaching/reattaching
physically, all at the same time. (I also tested ping and data
transfer too! But mostly this patch set affects the init/stop paths,
not the tx/rx paths.) So far I haven't won the game of `crash or
deadlock the system' this way!
It wouldn't hurt to test more of those devices, of course, but I also
don't have (working) devices handy for the following:
axe(4), axen(4), cdce(4), cue(4), kue(4), mos(4), mue(4), smsc(4),
udav(4), upl(4), urndis(4)
DEADLOCK. The deadlock happens when a device such as ure(4) is
unplugged:
1. usbnet_detach holds the usbnet core lock, awaits softclock `lock'
(i.e., softints running sequentially at each level on a single CPU)
for callout to wake kpause in ure_reset
2. callout holds softclock `lock', awaits softnet_lock in
tcp_timer_keep, frag6_fasttimo, &c.
3. soclose holds softnet_lock, awaits usbnet core lock in ure_ioctl
for SIOCDELMULTI
This patch set breaks the deadlock by handling hardware multicast
filter updates for SIOCADDMULTI/SIOCDELMULTI through a new usbnet
driver operation, uno_mcast, rather than through uno_ioctl, which
doesn't need the usbnet core lock or IFNET_LOCK.
The job of uno_mcast is to commit the ethersubr(9) multicast address
list to any hardware filter. usbnet(9) serializes calls to uno_mcast,
and only while the interface is up. After uno_stop, SIOCADDMULTI and
SIOCDELMULTI will still update the ethersubr(9) list, but usbnet(9)
won't bother calling uno_mcast until the next if_init, at which time
it will atomically call uno_mcast and begin accepting concurrent
multicast updates.
Note: uno_mcast is issued synchronously. However, the patch set makes
it asynchronous temporarily, so we can pull it up to netbsd-9, because
a delayed multicast filter update is better than a deadlock; later in
the patch set the synchronous updates are restored with a different
locking ABI.
(Could the deadlock have been done more simply, by teaching the
various *_uno_ioctl routines to not take the usbnet core lock for
SIOCADDMULTI/SIOCDELMULTI? They could do that -- but then they race
with init/stop, which is a whole nother can of worms. This patch set
addresses that in a much more regimented flow of driver callbacks.)
API CHANGES. There is now a rigid timeline on which usbnet(9) issues
driver callbacks (time flows downward; comma means `may happen in
parallel'; => means `invoked synchronously as a subroutine'):
usbnet_attach
(driver has exclusive access to everything here, no locking needed)
usbnet_attach_ifp
uno_ioctl, uno_read_reg or uno_write_reg or uno_statchg
ifp->if_init
=> uno_init
=> uno_mcast
uno_tx_prepare, uno_rx_loop, uno_tick, uno_intr, uno_ioctl, uno_mcast,
uno_read_reg or uno_write_reg or uno_statchg
ifp->if_stop
=> uno_stop
uno_ioctl, uno_read_reg or uno_write_reg or uno_statchg
usbnet_detach
Notes:
- Repeated if_init while IFF_RUNNING will be ignored, so uno_init is
only called on the interface-up transition from !IFF_RUNNING to
IFF_RUNNING, and not again until after stopping.
- Similarly, repeated if_stop while !IFF_RUNNING will be ignored, so
uno_stop is only called on the interface-down transition from
IFF_RUNNING to !IFF_RUNNING (and then only when the device is not
detaching -- there's no point in trying to write device registers to
reset the hardware if it's physically gone!), and not again until
after reinitialization.
- uno_init, uno_stop, and uno_ioctl are always called with IFNET_LOCK
held, the long-term interface configuration lock.
- Each of
. uno_tx_prepare
. uno_rx_loop
. uno_tick
. uno_intr
. uno_ioctl
. uno_mcast
. uno_read_reg or uno_write_reg or uno_statchg
independently will only ever be called in serial -- that is, e.g.,
there will never be two concurrent calls to uno_mcast for the same
usbnet device, or two concurrent calls to uno_rx_loop. But there
may be concurrent calls to uno_rx_loop and uno_mcast -- one of each.
- The MII callbacks uno_read_reg, uno_write_reg, and uno_statchg are
serialized among themselves -- there is never an uno_read_reg and
uno_write_reg at the same time on the same usbnet device, for
instance. However, they may run in parallel with anything else,
except uno_init or uno_stop, at any time between usbnet_attach_ifp
and usbnet_detach, irrespective of whether the interface is up or
down.
API DELETIONS. The following usbnet API functions are now gone:
- usbnet_set_dying -- only one driver, url(4), ever used this, and
only on attach failure, which usbnet already gracefully handles
- usbnet_lock_{core,tx,rx} &c. -- these locks are now internal to
usbnet.c; drivers have no need to take them
- usbnet_busy, usbnet_unbusy -- nothing relied on usbnet reference
counting; usbnet waits for the users of all resources to drain by
some other mechanism -- callout_halt, usb_rem_task_wait, if_detach,
&c. -- before destroying them, following the rigid timeline.
- usbnet_init_rx_tx -- now handled internally by usbnet's if_init
function; this only contained driver-independent logic to set up
pipes and callouts and, after changes for the hardware multicast
filter updates, was used the same way by all drivers as the last
thing done by uno_init.
- usbnet_stop -- now handled internally by usbnet's if_stop function;
contained driver-independent logic to tear down pipes and callouts,
along with a call to uno_stop. The driver-independent logic now
rigidly follows the timeline above; there is no need for drivers to
invoke it directly.
BUG IN MUE(4). Currently it is possible for uno_ioctl and uno_mcast to
run concurrently. This means there is a bug in mue(4), because they
touch the same registers. Two alternatives are:
1. Make uno_ioctl take the same lock inside usbnet as uno_mcast.
=> I'm reluctant because I don't know if some ioctls might
reasonably be blocking, which is not allowed for
SIOCADDMULTI/SIOCDELMULTI to wait for. But currently I don't
think this poses a problem with the usbnet drivers in tree --
I'm just worried about deadlocks introduced by future drivers
with fancier ioctls.
2. Create a new lock inside mue(4) for mue_uno_cast and
mue_sethwcsum_locked.
=> Makes things a little more complicated in the driver -- but the
complication of additional interactions is limited to this
driver, so maybe not so bad.
Preferences?
BUG IN SMSC(4). Currently there is a minor bug in smsc(4) because
smsc_setoe_locked, which services SIOCSIFCAP to enable or disable
hardware checksumming, races with smsc_uno_rx_loop over
sc->sc_coe_ctrl. Here are two way we might address this:
1. Create a lock that smsc_setoe_locked and smsc_uno_rx_loop agree on
to cover sc->sc_coe_ctrl or manage it with atomics, and if the rx
already happened (because the hardware packet delivery isn't
beholden to any kmutex_t we create), just accept that we might lose
some packets to checksum errors.
2. Make SIOCSIFCAP take the interface down, set the checksum cap, and
then bring the interface back up again.
I'm inclined to prefer option (1); option (2) means we'll still drop
some packets -- probably more! -- and it's more intrusive into the
flow of logic.
Preferences?