Bug 241489 - netmap + if_vlan panics related to 'Widen NET_EPOCH coverage' work (r353292)
Summary: netmap + if_vlan panics related to 'Widen NET_EPOCH coverage' work (r353292)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: amd64 Any
: --- Affects Some People
Assignee: Vincenzo Maffione
URL:
Keywords: crash, regression
Depends on:
Blocks:
 
Reported: 2019-10-25 14:11 UTC by Aleksandr Fedorov
Modified: 2019-11-02 01:57 UTC (History)
5 users (show)

See Also:


Attachments
Proposed fix (667 bytes, patch)
2019-10-26 10:27 UTC, Aleksandr Fedorov
no flags Details | Diff
Enter/exit epoch just once for the whole tx batch. (857 bytes, patch)
2019-10-26 20:37 UTC, Vincenzo Maffione
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aleksandr Fedorov freebsd_committer 2019-10-25 14:11:55 UTC
We widely use the configuration: vm (bhyve) - vale switch - if_vlan.
After r353292 we observe the following panic:

Unread portion of the kernel message buffer:
panic: Assertion in_epoch(net_epoch_preempt) failed at /afedorov/vstack-develop-freebsd/sys/net/if_vlan.c:1142
cpuid = 11
time = 1572003077
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe01d541d1e0
vpanic() at vpanic+0x17e/frame 0xfffffe01d541d240
panic() at panic+0x43/frame 0xfffffe01d541d2a0
vlan_transmit() at vlan_transmit+0x165/frame 0xfffffe01d541d2f0
nm_os_generic_xmit_frame() at nm_os_generic_xmit_frame+0x7c/frame 0xfffffe01d541d310
generic_netmap_txsync() at generic_netmap_txsync+0x2d6/frame 0xfffffe01d541d3e0
netmap_bwrap_notify() at netmap_bwrap_notify+0x96/frame 0xfffffe01d541d410
nm_vale_flush() at nm_vale_flush+0xa53/frame 0xfffffe01d541d510
netmap_vale_vp_txsync() at netmap_vale_vp_txsync+0x508/frame 0xfffffe01d541d5a0
netmap_ioctl() at netmap_ioctl+0x1b4/frame 0xfffffe01d541d670
freebsd_netmap_ioctl() at freebsd_netmap_ioctl+0x88/frame 0xfffffe01d541d6b0
devfs_ioctl() at devfs_ioctl+0xca/frame 0xfffffe01d541d700
VOP_IOCTL_APV() at VOP_IOCTL_APV+0x85/frame 0xfffffe01d541d720
vn_ioctl() at vn_ioctl+0x13d/frame 0xfffffe01d541d830
devfs_ioctl_f() at devfs_ioctl_f+0x1e/frame 0xfffffe01d541d850
kern_ioctl() at kern_ioctl+0x295/frame 0xfffffe01d541d8b0
sys_ioctl() at sys_ioctl+0x15c/frame 0xfffffe01d541d980
amd64_syscall() at amd64_syscall+0x2b5/frame 0xfffffe01d541dab0
fast_syscall_common() at fast_syscall_common+0x101/frame 0xfffffe01d541dab0
--- syscall (54, FreeBSD ELF64, sys_ioctl), rip = 0x800819c2a, rsp = 0x7fffdfffce68, rbp = 0x7fffdfffcef0 ---
Uptime: 19m12s
Dumping 5033 out of 65374 MB:..1%..11%..21%..31%..41%..51%..61%..71%..81%..91%

The problem is that NET_EPOCH_ENTER() is moved out of the vlan_transmit() function. But the netmap generic code calls the vlan_transmit () function directly from nm_os_generic_xmit_frame (), without entering to the EPOCH section. So, kernel panics on NET_EPOCH_ASSERT().

Temporarily, I solved the problem with the following patch, but I'm not sure if this is the correct solution.

--- a/sys/dev/netmap/netmap_freebsd.c
+++ b/sys/dev/netmap/netmap_freebsd.c
@@ -420,6 +420,7 @@ nm_os_generic_xmit_frame(struct nm_os_gen_arg *a)
 {
        int ret;
        u_int len = a->len;
+       struct epoch_tracker et;
        struct ifnet *ifp = a->ifp;
        struct mbuf *m = a->m;
 
@@ -453,9 +454,11 @@ nm_os_generic_xmit_frame(struct nm_os_gen_arg *a)
        M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE);
        m->m_pkthdr.flowid = a->ring_nr;
        m->m_pkthdr.rcvif = ifp; /* used for tx notification */
+       NET_EPOCH_ENTER(et);
        CURVNET_SET(ifp->if_vnet);
        ret = NA(ifp)->if_transmit(ifp, m);
        CURVNET_RESTORE();
+       NET_EPOCH_EXIT(et);
        return ret ? -1 : 0;
 }
Comment 1 Hans Petter Selasky freebsd_committer 2019-10-25 14:53:36 UTC
This patch looks correct.

All if_transmit() functions should be under EPOCH (13-current) if I understand Gleb correctly.

--HPS
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-26 01:32:13 UTC
@Aleksandr Could you include your proposed patch as an attachment please
Comment 3 Aleksandr Fedorov freebsd_committer 2019-10-26 10:27:38 UTC
Created attachment 208604 [details]
Proposed fix

Add proposed patch to attachments.
Comment 4 Vincenzo Maffione freebsd_committer 2019-10-26 12:59:13 UTC
Thanks for the patch.
If it is true that all the if_transmit() invocations must happen under the net epoch, then the patch is correct.
I ran the netmap unit tests and integration tests under this patch --> ok.
Comment 5 Vincenzo Maffione freebsd_committer 2019-10-26 20:37:00 UTC
Created attachment 208617 [details]
Enter/exit epoch just once for the whole tx batch.
Comment 6 Vincenzo Maffione freebsd_committer 2019-10-26 20:37:30 UTC
In any case, it is enough to enter the epoch just once for the whole batch of tx packets rather than entering/exiting for each transmission.
This would mean better performance.
Could you please try the attached patch?
Comment 7 Aleksandr Fedorov freebsd_committer 2019-10-27 15:51:59 UTC
(In reply to Vincenzo Maffione from comment #6)

I can’t check the difference in performance right now. But the network works with the new patch (iperf3, ping -f).
Comment 8 Vincenzo Maffione freebsd_committer 2019-10-27 16:16:42 UTC
That's ok. The performance difference may be irrelevant in your setup, or maybe your average tx batch is very small (or even 1).
In any case, amortizing the epoch(9) operation over the whole batch is the optimal approach, so I will commit this change.
Thanks for testing.
Comment 9 commit-hook freebsd_committer 2019-10-28 19:00:33 UTC
A commit references this bug:

Author: vmaffione
Date: Mon Oct 28 19:00:27 UTC 2019
New revision: 354137
URL: https://svnweb.freebsd.org/changeset/base/354137

Log:
  netmap: enter NET_EPOCH on generic txsync

  After r353292, netmap generic adapter on if_vlan interfaces panics on
  asserting the NET_EPOCH. In more detail, this happens when
  nm_os_generic_xmit_frame() is called, that is in the generic txsync
  routine.
  Fix the issue by entering the NET_EPOCH during the generic txsync.
  We amortize the cost of entering/exiting over a whole batch of
  transmissions.

  PR:		241489
  Reported by:	Aleksandr Fedorov <aleksandr.fedorov@itglobal.com>

Changes:
  head/sys/dev/netmap/netmap_generic.c
Comment 10 Vincenzo Maffione freebsd_committer 2019-10-30 16:18:22 UTC
Can we close this?