@mmacy : Can you implement a function in the EPOCH API to drain all pending EPOCH callbacks? And take over this patch and commit a proper fix?

The epoch_call_drain() function is indeed needed (at least to fix such panic https://reviews.freebsd.org/F4491011).
But your example shows that epoch based reclamation is just wrongly used. The right solution should be keeping ifnet detached until all possible consumers stop reference it, and only then it will be safe to free ifnet pointer.

In D20109#432627, @ae wrote:

The epoch_call_drain() function is indeed needed (at least to fix such panic https://reviews.freebsd.org/F4491011).
But your example shows that epoch based reclamation is just wrongly used. The right solution should be keeping ifnet detached until all possible consumers stop reference it, and only then it will be safe to free ifnet pointer.

Using EPOCH to do this is faster than touching an atomic and global refcount on the ifnet structure.

I feel like there's a better way to do this, but haven't come up with a solution yet.

@hselasky this is completely contrary to the intent of epoch - what callbacks are you seeing executed after a destroy?

@mmacy: The epoch_call() is not covered by epoch_wait_preempt(). We simply see deferred epoch calls being executed after the the IFP has been destroyed. Typically the multicast destroy ones.

In D20109#434039, @hselasky wrote:

@mmacy: The epoch_call() is not covered by epoch_wait_preempt(). We simply see deferred epoch calls being executed after the the IFP has been destroyed. Typically the multicast destroy ones.

You need to bump the refcount before the epoch_call. That's how this is handled elsewhere.

@mmacy : The e_drain_count is incremented in the previous for loop. Which refcount are you referring to?

I also see that as contrary to intent of epoch.

How exactly we dereference the ifnet being freed leading to panic? Is there some sort of asynchronous destruction callback in multicast code?

@glebius: Multicast destruction is deferred. When we destroy a multicast address we need to call the if_ioctl of the belonging network interface to remove any multicast addresses. That's the problem. I think draining is a good way to implement a safe solution instead of using refcounts. Then we ensure that the ifnet is in a certain state when the multicast destruction callbacks are invoked.

In D20109#437264, @hselasky wrote:

@glebius: Multicast destruction is deferred. When we destroy a multicast address we need to call the if_ioctl of the belonging network interface to remove any multicast addresses. That's the problem. I think draining is a good way to implement a safe solution instead of using refcounts. Then we ensure that the ifnet is in a certain state when the multicast destruction callbacks are invoked.

Personally, I would prefer the refcount approach outlined by @mmacy. Can you explain why that is insufficient? Is multicast address destruction something that happens commonly enough that it is so highly performance sensitive that the atomics are a performance bottleneck?

@jtl: The refcount approach is not insufficient. We can do both ways, only that the synchronous approach is more safe-playing in my opinion and it is not that expensive and doesn't need additional checking in the multicast deferred destruction code-path for NULL pointers and destroyed resources, which the refcount approach will require.

@jtl: Imagine the following scenario.

Network drivers typically free their if_softc after if_free(). But deferred multicast epoch callbacks may still be running inside the driver if_ioctl() handler.

I don't see any other way than to make a synchronous barrier, because network drivers softc's are not refcounted like the ifnet softc and that is out of our control.

Refcounting everything is currently impractical like mandated by EPOCH, so a synchronous barrier may be a the best way forward here!

Ping.