if_bridge(4): don't sleep under epoch

[pouria]

Summary

bridge tries to run callout_drain twice
under epoch during destruction.
once for bridge_timer, which is not required to be under epoch.
considering it's stopped at the first line of that function.
and the second time for bstp callout. which is already disabled
earlier inside the bridge_delete_member.
There for there is no need to be under epoch to protect those operations.

This patch fixes witness warnings below:

calling _callout_stop_safe with 1 sleep inhibitors
stack backtrace:
#0 0xffffffff80c13c5c at witness_debugger+0x6c
#1 0xffffffff80c15535 at witness_warn+0x4a5
#2 0xffffffff80bba963 at _callout_stop_safe+0x73
#3 0xffffffff836114e5 at bridge_clone_destroy+0x135
#4 0xffffffff80ce1cbd at if_clone_destroyif_flags+0x6d
#5 0xffffffff80ce1bbd at if_clone_destroy+0xfd
#6 0xffffffff80cdd1c7 at ifioctl+0x437
#7 0xffffffff80c1a977 at kern_ioctl+0x247
#8 0xffffffff80c1a6c7 at sys_ioctl+0x137
#9 0xffffffff810dc795 at amd64_syscall+0x175
#10 0xffffffff810ac96b at fast_syscall_common+0xf8

calling _callout_stop_safe with 1 sleep inhibitors
stack backtrace:
#0 0xffffffff80c13c5c at witness_debugger+0x6c
#1 0xffffffff80c15535 at witness_warn+0x4a5
#2 0xffffffff80bba963 at _callout_stop_safe+0x73
#3 0xffffffff8361c751 at bstp_detach+0xa1
#4 0xffffffff83611596 at bridge_clone_destroy+0x1e6
#5 0xffffffff80ce1cbd at if_clone_destroyif_flags+0x6d
#6 0xffffffff80ce1bbd at if_clone_destroy+0xfd
#7 0xffffffff80cdd1c7 at ifioctl+0x437
#8 0xffffffff80c1a977 at kern_ioctl+0x247
#9 0xffffffff80c1a6c7 at sys_ioctl+0x137
#10 0xffffffff810dc795 at amd64_syscall+0x175
#11 0xffffffff810ac96b at fast_syscall_common+0xf8

Test Plan

kyua test -k /usr/tests/Kyuafile sys/net

[glebius]

Thanks!

[zlei]

sys/net/if_bridge.c

964

Emm, the bridge_clone_destroy() is a typical control path, and it is tearing down the driver private data and ifnet, so it should have its own means to avoid racing. I do not expect the net epoch is required here.

970

The ether_ifdetach() appears to be too late to me. Typically the ifnet should be unlinked from the "active" list first, and then it is safe to cleanup the driver private data.

[pouria]

sys/net/if_bridge.c

964

I'm not sure about removing epoch side-effects on IFQ_PURGE.
I prefer to limit scope of this change to fixing the witness warnings.
But I'll work on that too.

[zlei]

sys/net/if_bridge.c

964

OK, let's proceed.

[pouria]

sys/net/if_bridge.c

964

I think IFQ_PURGE requires the epoch enter.
See _IF_DEQUEUE macro in sys/net/ifq.h.

What do you think?
should we remove the epoch enter/exit entirely?

FYI: for now we can't remove the defered epoch call for bridge_clone_destroy_cb:
See top of the if_bridge.c:

206 /*
207  * Bridge locking
208  *
209  * The bridge relies heavily on the epoch(9) system to protect its data
210  * structures. This means we can safely use CK_LISTs while in NET_EPOCH, but we
211  * must ensure there is only one writer at a time.
212  *
213  * That is: for read accesses we only need to be in NET_EPOCH, but for write
214  * accesses we must hold:
215  *
216  *  - BRIDGE_RT_LOCK, for any change to bridge_rtnodes
217  *  - BRIDGE_LOCK, for any other change
218  *
219  * The BRIDGE_LOCK is a sleepable lock, because it is held across ioctl()
220  * calls to bridge member interfaces and these ioctl()s can sleep.
221  * The BRIDGE_RT_LOCK is a non-sleepable mutex, because it is sometimes
222  * required while we're in NET_EPOCH and then we're not allowed to sleep.
223  */

[zlei]

sys/net/if_bridge.c

966

IFQ_PURGE() will hold the mutex lock, before calling _IF_DEQUEUE().

#define IFQ_PURGE(ifq)                                                  \
do {                                                                    \
        IF_LOCK(ifq);                                                   \
        IFQ_PURGE_NOLOCK(ifq);                                          \
        IF_UNLOCK(ifq);                                                 \
} while (0)

#define IF_LOCK(ifq)            mtx_lock(&(ifq)->ifq_mtx)

And ifq_mtx is a blocking mutex...

[pouria]

sys/net/if_bridge.c

966

I understand, but I'm worried about the mbufs in the queue, they may need epoch.
of course, it tries to dequeue them.
Let me test the alq without epoch in the lab to make sure nothing wrong happens.
Then I will create a new revision to fix that too.
Thank you again.

[glebius]

mbufs on any queue shall not need epoch.