In D12086#258234, @mav wrote:

Do you think the case when lower and orphan hooks are connected to the same node is realistic enough to make this code so tangled?

In D12086#258234, @mav wrote:

While I understand your valid motivation, I can't say that I very much like this in patch for several reasons:

• NGM_ETHER_SET_ENADDR is a control command supposed to be targeted to ng_ether node, not from it, and it is a action request, not notification.

Expand a comment explaining code.

PS: Read how to create patches with context for the phabricator. May be it is a problem of phabricator and it modified the diff after you, if I download the raw diff here, there is no even normal diff -p context, which is almost always useful.

While I understand your valid motivation, I can't say that I very much like this in patch for several reasons:

• NGM_ETHER_SET_ENADDR is a control command supposed to be targeted to ng_ether node, not from it, and it is a action request, not notification. I suppose if you connect two ng_ether nodes with lower hooks to create poor mans bridge, this won't work as expected.
• ng_pppoe has method to explicitly set MAC address, and it may get overwritten by this, which may or may not be correct.
• We just fought one locking issue, and here I don't see any hook locks or even references (I understand that your problem is narrower, but still).

Remove extra directory adds

Not sure where those head/* directories came from...

Update my tree before updating revision

incorporate some missing changes from dev branch

Commit was reverted

In D12101#251708, @sbruno wrote:

Huh .... Testing this review by itself this morning. I see the following startup panic:

Sleeping on "e1000_delay" with the following non-sleepable locks held:
exclusive sleep mutex em2:tx(0):callo (em2:tx(0):callo) r = 0 (0xfffff80004175068) locked @ /home/sbruno/bsd/fbsd_head/sys/kern/kern_mutex.c:182
stack backtrace:
#0 0xffffffff80ac85d3 at witness_debugger+0x73
#1 0xffffffff80ac99cf at witness_warn+0x43f
#2 0xffffffff80a708fc at _sleep+0x6c
#3 0xffffffff80a71117 at pause_sbt+0x117
#4 0xffffffff805620e1 at e1000_read_phy_reg_mdic+0xf1
#5 0xffffffff80562b5b at e1000_read_phy_reg_igp+0x5b
#6 0xffffffff80535d6c at em_if_timer+0xcc
#7 0xffffffff80b76279 at iflib_timer+0x69
#8 0xffffffff80a7e375 at softclock_call_cc+0x155
#9 0xffffffff80a7e75c at softclock+0x7c
#10 0xffffffff80a2bed1 at intr_event_execute_handlers+0x91
#11 0xffffffff80a2c5d6 at ithread_loop+0xb6
#12 0xffffffff80a29304 at fork_exit+0x84
#13 0xffffffff80ecf7ce at fork_trampoline+0xe
panic: sleepq_add: td 0xfffff80003d59000 to sleep on wchan 0xffffffff81c7d260 with sleeping prohibited
cpuid = 0
time = 4
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00f55f4730
vpanic() at vpanic+0x19c/frame 0xfffffe00f55f47b0
kassert_panic() at kassert_panic+0x126/frame 0xfffffe00f55f4820
sleepq_add() at sleepq_add+0x34f/frame 0xfffffe00f55f4870
_sleep() at _sleep+0x26c/frame 0xfffffe00f55f4910
pause_sbt() at pause_sbt+0x117/frame 0xfffffe00f55f4960
e1000_read_phy_reg_mdic() at e1000_read_phy_reg_mdic+0xf1/frame 0xfffffe00f55f49b0
e1000_read_phy_reg_igp() at e1000_read_phy_reg_igp+0x5b/frame 0xfffffe00f55f49e0
em_if_timer() at em_if_timer+0xcc/frame 0xfffffe00f55f4a10
iflib_timer() at iflib_timer+0x69/frame 0xfffffe00f55f4a40
softclock_call_cc() at softclock_call_cc+0x155/frame 0xfffffe00f55f4af0
softclock() at softclock+0x7c/frame 0xfffffe00f55f4b20
intr_event_execute_handlers() at intr_event_execute_handlers+0x91/frame 0xfffffe00f55f4b60
ithread_loop() at ithread_loop+0xb6/frame 0xfffffe00f55f4bb0
fork_exit() at fork_exit+0x84/frame 0xfffffe00f55f4bf0
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe00f55f4bf0
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---
KDB: enter: panic
[ thread pid 12 tid 100023 ]
Stopped at      kdb_enter+0x3b: movq    $0,kdb_why

move more blocking work to deferred context

Yeah, legacy em(4) device on em2:

Huh .... Testing this review by itself this morning. I see the following startup panic:

In D12101#251537, @kmacy wrote:

In D12101#251488, @erj wrote:

In D12101#251426, @johalun0_gmail.com wrote:

In D12101#251279, @johalun0_gmail.com wrote:

The busy waiting would cause 10% CPU usage on my system when no cable connected to em0 (traced to em_if_update_admin_status).
This patch also fixes this. I can no longer detect any unusual CPU usage.

Getting a lot of witness output. Like a few per second or so.

I think this revision depends on D11969. It changes the core lock from a mutex to an sx lock, which would allow sleeping while its held.

Actually the problem is that I didn't completely eliminate the use of mutexes in the OS independent code. Update forthcoming.

• Eliminate remaining mutex use in os independent e1000 code.
• Fold in iflib updates.

In D12101#251488, @erj wrote:

In D12101#251426, @johalun0_gmail.com wrote:

In D12101#251279, @johalun0_gmail.com wrote:

The busy waiting would cause 10% CPU usage on my system when no cable connected to em0 (traced to em_if_update_admin_status).
This patch also fixes this. I can no longer detect any unusual CPU usage.

Getting a lot of witness output. Like a few per second or so.

I think this revision depends on D11969. It changes the core lock from a mutex to an sx lock, which would allow sleeping while its held.

In D12101#251426, @johalun0_gmail.com wrote:

In D12101#251279, @johalun0_gmail.com wrote:

The busy waiting would cause 10% CPU usage on my system when no cable connected to em0 (traced to em_if_update_admin_status).
This patch also fixes this. I can no longer detect any unusual CPU usage.

Getting a lot of witness output. Like a few per second or so.

In D12101#251279, @johalun0_gmail.com wrote:

The busy waiting would cause 10% CPU usage on my system when no cable connected to em0 (traced to em_if_update_admin_status).
This patch also fixes this. I can no longer detect any unusual CPU usage.

@jtl any comments?

@erj I updated the summary - is there anything more I should add?

Looks good.

The busy waiting would cause 10% CPU usage on my system when no cable connected to em0 (traced to em_if_update_admin_status).
This patch also fixes this. I can no longer detect any unusual CPU usage.

In D12101#251145, @kmacy wrote:

In D12101#251141, @erj wrote:

Can we also get a description of what this fixes, for future reference?

Two problems:

• As we've discussed at length elsewhere, Intel drivers will busy wait for arbitrary lengths of time with a default mutex held when waiting for the firmware to release a resource.
• There is a lot of mostly duplicated code with tiny (dubious) variations.

With this patch, and kern.ipc.cachespread="1", network traffic stops after some indeterminate time.

In D12101#251141, @erj wrote:

Can we also get a description of what this fixes, for future reference?

Can we also get a description of what this fixes, for future reference?

update to reflect potential for brokenness

I'd say go ahead for now; seems to make life better at least.

I've done a bit more testing, and these debug traces tell the story:

Hmm, good question. I thought I understood the failure flow fully, but now I'm not so sure.

I think your description could be a bit more clear as I got confused. I think after re-reading it I now understand what you mean. Can you try to describe the sequence of events step by step here on how this gets triggered (especially how the new entry appears before we set it to NULL overwriting a new one?)?