Page MenuHomeFreeBSD
Differential D15355

lagg: Remove redundant rmlock
ClosedPublic
Actions

Authored by shurd on May 8 2018, 6:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 2, 12:26 AM
Unknown Object (File)
Sat, Nov 30, 4:04 PM
Unknown Object (File)
Fri, Nov 29, 9:23 PM
Unknown Object (File)
Mon, Nov 25, 4:23 PM
Unknown Object (File)
Sat, Nov 23, 9:31 PM
Unknown Object (File)
Sat, Nov 23, 2:04 PM
Unknown Object (File)
Sat, Nov 23, 4:17 AM
Unknown Object (File)
Fri, Nov 22, 3:09 PM
View All Files
Subscribers
ae
dumbbell
imp

Details

Reviewers
sbruno
mmacy
mav
glebius
adrian
melifaro
Commits
rS333612: Replace rmlock with epoch in lagg
Summary

LAGG_WLOCK was completely contained inside LAGG_XLOCK, and
the rmlock is triggering LORs in the multicast stuff. Switch LAGG_RLOCK
to LAGG_SLOCK.

Test Plan

Test for performance regressions and check for LAGG_SLOCK recursion.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16503
Build 16417: arc lint + arc unit

Event Timeline

shurd created this revision.May 8 2018, 6:55 PM
Herald added subscribers: ae, imp. · View Herald TranscriptMay 8 2018, 6:55 PM
Harbormaster completed remote builds in B16481: Diff 42280.May 8 2018, 6:55 PM
shurd updated this revision to Diff 42282.May 8 2018, 7:12 PM

Rather than remove LAGG_WLOCK_ASSERT()s, convert to LAGG_XLOCK_ASSERT()

Harbormaster completed remote builds in B16482: Diff 42282.May 8 2018, 7:12 PM
mmacy added inline comments.May 8 2018, 7:19 PM
sys/net/if_lagg.c
1759

I don't think we want to be acquiring sleep locks in the data path. A more substantive refactoring is in order.

mmacy added a comment.May 8 2018, 7:21 PM

What we need to do is not be traversing the list in the data path and just hold a reference from an array to all the ports. At that point we're safe just using an sx lock. I think we can handle this with EBR + SX. I'll hack something up with subr_epoch

shurd added a comment.May 8 2018, 8:00 PM

Yeah, all the LAGG_RLOCK -> LAGG_SLOCK conversions need to be fixed up... right now it's a LOR generator for sure.

shurd updated this revision to Diff 42293.May 8 2018, 10:36 PM

Ressurect the rwlock, but only to protect the new sc_slowpath boolean.
If the slowpath is currently running, DELAY(1) until it's not. Still a
bit of a hack, but now it doesn't (shouldn't) cause LORs.

Harbormaster completed remote builds in B16486: Diff 42293.May 8 2018, 10:36 PM
shurd marked an inline comment as done.May 8 2018, 10:37 PM
shurd updated this revision to Diff 42294.May 8 2018, 10:43 PM

Protect against multiple slowpath entries.

Harbormaster completed remote builds in B16487: Diff 42294.May 8 2018, 10:43 PM
shurd updated this revision to Diff 42324.May 9 2018, 5:26 PM

Use bool and true/false rather than _Bool and 1/0

Harbormaster completed remote builds in B16499: Diff 42324.May 9 2018, 5:26 PM
melifaro requested changes to this revision.May 9 2018, 7:48 PM

Would you mind describe the proposed locking model somewhere in the file explicitly?
In particular, 1) what does _XLOCK or _SLOCK locks protect? 2) How does the sc_slowpath work? 3) What is "right" lock order?
It would be beneficial to describe some examples of problematic potential LORs as well.

sys/net/if_lagg.c
1759

Why do you think it is possible to have DELAY(1) on the datapath?

sys/net/if_lagg.h
263–264

These primitives seem to be used in the control path. Why not convert them to functions?
This kind of spaghetti is unmaintainable.

This revision now requires changes to proceed.May 9 2018, 7:48 PM
shurd updated this revision to Diff 42337.May 9 2018, 9:39 PM

Address feedback.

Harbormaster completed remote builds in B16503: Diff 42337.May 9 2018, 9:39 PM
shurd marked 2 inline comments as done.May 9 2018, 9:41 PM
shurd added inline comments.
sys/net/if_lagg.c
1759

Fixed (lagg_rlock() uses cpu_spinwait() now).

sys/net/if_lagg.h
263–264

Done. I also did the lagg_rlock and lagg_runlock() as inline.

shurd marked 2 inline comments as done.May 9 2018, 9:42 PM
shurd updated this revision to Diff 42338.May 9 2018, 9:49 PM

Destroy rm lock, clean up spurious changes.

Harbormaster completed remote builds in B16504: Diff 42338.May 9 2018, 9:49 PM
shurd updated this revision to Diff 42381.May 10 2018, 7:48 PM

Use the new epoch based reclamation API. Now the hot paths will not
block at all, and the sx lock is used for the softc data.

Obtained from mmacy@, I'm testing now.

Harbormaster completed remote builds in B16522: Diff 42381.May 10 2018, 7:48 PM
mmacy added a comment.May 10 2018, 8:08 PM

epoch(9) documentation is forthcoming but in the meantime some clarification of semantics:

epoch does not have lock ordering issues its only restrictions are:

  • While in an epoch section one can't do a preemptible epoch_enter on a different epoch_t
  • No sleeping or sx lock acquisition in an epoch section
  • One must use safe list traversal and removal routines
  • Object destruction must follow a grace period
sbruno accepted this revision.May 13 2018, 2:30 PM

dumbbell@ has reported that this fixes his current problems with lagg(4) panicing on his hosts.

shurd updated this revision to Diff 42539.May 14 2018, 6:28 PM

Update patch to latest head

Harbormaster completed remote builds in B16587: Diff 42539.May 14 2018, 6:28 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 14 2018, 8:06 PM
Closed by commit rS333612: Replace rmlock with epoch in lagg (authored by shurd). · Explain Why
This revision was automatically updated to reflect the committed changes.
dumbbell added a subscriber: dumbbell.May 14 2018, 9:59 PM

Thank you for the quick fix!

Since then I hit two panics on two laptops with the same kernel, but I'm not sure they are related to lagg(4).

The first one where lagg(4) is used with one em(4) only underneath:

Fatal trap 12: page fault while in kernel mode
cpuid = 7; apic id = 07
fault virtual address   = 0x10
fault code              = supervisor read data, page not present
instruction pointer     = 0x20:0xffffffff80d1c876
stack pointer           = 0x28:0xfffffe000042d650
frame pointer           = 0x28:0xfffffe000042d670
code segment            = base 0x0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags        = interrupt enabled, resume, IOPL = 0
current process         = 0 (if_io_tqg_7)
trap number             = 12
panic: page fault
cpuid = 7
time = 1526247877
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe000042d300
vpanic() at vpanic+0x1a3/frame 0xfffffe000042d360
panic() at panic+0x43/frame 0xfffffe000042d3c0
trap_fatal() at trap_fatal+0x35f/frame 0xfffffe000042d410
trap_pfault() at trap_pfault+0x49/frame 0xfffffe000042d470
trap() at trap+0x2e3/frame 0xfffffe000042d580
calltrap() at calltrap+0x8/frame 0xfffffe000042d580
--- trap 0xc, rip = 0xffffffff80d1c876, rsp = 0xfffffe000042d650, rbp = 0xfffffe000042d670 ---
inm_lookup() at inm_lookup+0x46/frame 0xfffffe000042d670
igmp_input() at igmp_input+0x48f/frame 0xfffffe000042d730
ip_input() at ip_input+0x145/frame 0xfffffe000042d790
netisr_dispatch_src() at netisr_dispatch_src+0xd6/frame 0xfffffe000042d7e0
ether_demux() at ether_demux+0x163/frame 0xfffffe000042d810
ether_nh_input() at ether_nh_input+0x363/frame 0xfffffe000042d870
netisr_dispatch_src() at netisr_dispatch_src+0xd6/frame 0xfffffe000042d8c0
ether_input() at ether_input+0x54/frame 0xfffffe000042d8f0
_task_fn_rx() at _task_fn_rx+0xaf0/frame 0xfffffe000042d9e0
gtaskqueue_run_locked() at gtaskqueue_run_locked+0x144/frame 0xfffffe000042da40
gtaskqueue_thread_loop() at gtaskqueue_thread_loop+0x98/frame 0xfffffe000042da70
fork_exit() at fork_exit+0x83/frame 0xfffffe000042dab0
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe000042dab0
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---
Uptime: 19s
Dumping 788 out of 16030 MB:..3%..11%..21%..31%..41%..51%..61%..72%..82%..92%

__curthread () at ./machine/pcpu.h:231
231     ./machine/pcpu.h: No such file or directory.
(kgdb) #0  __curthread () at ./machine/pcpu.h:231
#1  doadump (textdump=1)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:366
#2  0xffffffff80b9e1e0 in kern_reboot (howto=260)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:446
#3  0xffffffff80b9e673 in vpanic (fmt=<optimized out>, ap=0xfffffe000042d3a0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:863
#4  0xffffffff80b9e463 in panic (fmt=<unavailable>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:790
#5  0xffffffff810658bf in trap_fatal (frame=0xfffffe000042d590, eva=16)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/amd64/amd64/trap.c:880
#6  0xffffffff81065919 in trap_pfault (frame=0xfffffe000042d590, usermode=0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/amd64/amd64/trap.c:717
#7  0xffffffff810650f3 in trap (frame=0xfffffe000042d590)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/amd64/amd64/trap.c:418
#8  <signal handler called>
#9  inm_lookup_locked (ifp=<optimized out>, ina=...)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/netinet/in_mcast.c:349
#10 inm_lookup (ifp=<optimized out>, ina=...)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/netinet/in_mcast.c:368
#11 0xffffffff80d1476f in igmp_input_v2_report (ifp=<optimized out>, 
    igmp=<optimized out>, ip=<optimized out>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/netinet/igmp.c:1364
#12 igmp_input (mp=0xfffffe000042d760, offp=0xfffffe000042d75c, proto=2)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/netinet/igmp.c:1576
#13 0xffffffff80d29ad5 in ip_input (m=0x0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/netinet/ip_input.c:825
#14 0xffffffff80cbf286 in netisr_dispatch_src (proto=1, 
    source=<optimized out>, m=0xfffff8001cbd9ec0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/netisr.c:1122
#15 0xffffffff80ca4273 in ether_demux (ifp=0xfffff8002a974800, 
    m=0xfffff800035fb580)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/if_ethersubr.c:873
#16 0xffffffff80ca53f3 in ether_input_internal (ifp=0xfffff8002a974800, 
    m=0xfffff800035fb580)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/if_ethersubr.c:661
#17 ether_nh_input (m=<optimized out>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/if_ethersubr.c:691
#18 0xffffffff80cbf286 in netisr_dispatch_src (proto=5, 
    source=<optimized out>, m=0xfffff8001cbd9ec0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/netisr.c:1122
#19 0xffffffff80ca4674 in ether_input (ifp=0xfffff8000397a000, m=0x0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/if_ethersubr.c:781
#20 0xffffffff80cb7140 in iflib_rxeof (rxq=<optimized out>, 
    budget=<optimized out>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/iflib.c:2700
#21 _task_fn_rx (context=<optimized out>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/net/iflib.c:3750
#22 0xffffffff80be6904 in gtaskqueue_run_locked (queue=0xfffff800030bd700)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/subr_gtaskqueue.c:332
#23 0xffffffff80be6568 in gtaskqueue_thread_loop (arg=<optimized out>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/subr_gtaskqueue.c:507
#24 0xffffffff80b5f1c3 in fork_exit (
    callout=0xffffffff80be64d0 <gtaskqueue_thread_loop>, 
    arg=0xfffffe00047f90b0, frame=0xfffffe000042dac0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_fork.c:1039
#25 <signal handler called>

The second one where lagg(4) is used with one em(4) and one iwm(4):

Fatal trap 12: page fault while in kernel mode
cpuid = 4; apic id = 04
fault virtual address   = 0x8000000
fault code              = supervisor write data, page not present
instruction pointer     = 0x20:0xffffffff833ddabb
stack pointer           = 0x28:0xfffffe008c4fe800
frame pointer           = 0x28:0xfffffe008c4fe8e0
code segment            = base 0x0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags        = interrupt enabled, resume, IOPL = 0
current process         = 12 (irq277: iwm0)
trap number             = 12
panic: page fault
cpuid = 4
time = 1526332705
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe008c4fe4b0
vpanic() at vpanic+0x1a3/frame 0xfffffe008c4fe510
panic() at panic+0x43/frame 0xfffffe008c4fe570
trap_fatal() at trap_fatal+0x35f/frame 0xfffffe008c4fe5c0
trap_pfault() at trap_pfault+0x49/frame 0xfffffe008c4fe620
trap() at trap+0x2e3/frame 0xfffffe008c4fe730
calltrap() at calltrap+0x8/frame 0xfffffe008c4fe730
--- trap 0xc, rip = 0xffffffff833ddabb, rsp = 0xfffffe008c4fe800, rbp = 0xfffffe008c4fe8e0 ---
iwm_intr() at iwm_intr+0x10ab/frame 0xfffffe008c4fe8e0
intr_event_execute_handlers() at intr_event_execute_handlers+0xe9/frame 0xfffffe008c4fe920
ithread_loop() at ithread_loop+0xe7/frame 0xfffffe008c4fe970
fork_exit() at fork_exit+0x83/frame 0xfffffe008c4fe9b0
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe008c4fe9b0
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---
Uptime: 3s
Dumping 848 out of 16221 MB:..2%..12%..21%..31%..42%..51%..61%..72%..82%..91%

__curthread () at ./machine/pcpu.h:231
231             __asm("movq %%gs:%1,%0" : "=r" (td)
(kgdb) #0  __curthread () at ./machine/pcpu.h:231
#1  doadump (textdump=1)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:366
#2  0xffffffff80b9e1e0 in kern_reboot (howto=260)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:446
#3  0xffffffff80b9e673 in vpanic (fmt=<optimized out>, ap=0xfffffe008c4fe550)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:863
#4  0xffffffff80b9e463 in panic (fmt=<unavailable>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_shutdown.c:790
#5  0xffffffff810658bf in trap_fatal (frame=0xfffffe008c4fe740, eva=134217728)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/amd64/amd64/trap.c:880
#6  0xffffffff81065919 in trap_pfault (frame=0xfffffe008c4fe740, usermode=0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/amd64/amd64/trap.c:717
#7  0xffffffff810650f3 in trap (frame=0xfffffe008c4fe740)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/amd64/amd64/trap.c:418
#8  <signal handler called>
#9  iwm_intr (arg=<optimized out>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/dev/iwm/if_iwm.c:5667
#10 0xffffffff80b61f99 in intr_event_execute_handlers (p=<optimized out>, 
    ie=0xfffff800035b4400)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_intr.c:1338
#11 0xffffffff80b62687 in ithread_execute_handlers (ie=<optimized out>, 
    p=<optimized out>)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_intr.c:1351
#12 ithread_loop (arg=0xfffff8000a024160)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_intr.c:1432
#13 0xffffffff80b5f1c3 in fork_exit (
    callout=0xffffffff80b625a0 <ithread_loop>, arg=0xfffff8000a024160, 
    frame=0xfffffe008c4fe9c0)
    at /home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_fork.c:1039
#14 <signal handler called>

Both panics occured during boot, never after. Also, they occured once each in the past two days, despite numerous reboots.

Kernel is compiled from rS333556 and local modifications are:

  • the patch to lagg(4) from this review
  • the patch from D15070 (evdev)
  • the patch from D15302 (vt(4))

Revision Contents
Changeset List

PathSize
sys/
net/
12 lines
256 lines

Diff 42337

View Options

sys/net/if_lagg.h

Loading...
View Options

sys/net/if_lagg.c

Loading...