The problem is what you submitted is logically inconsistent in the e1000 code. Other consumers of this code (i.e. Linux, DPDK) have not plugged the holes either (for the record they do not implement this improvement). I've provided a starting pointer in the phy code. You will find at least one more issue in the lem(4) phy code. This is via ripgrep and code inspection, you don't need any hardware to see it.

I can see where the "shared code" (e1000 api) is not ready in general and for lem(4). For the submitter, if you want to try and chase that down e1000_setup_copper_link_generic() in e1000_phy.c.. maybe try modifying the conditional. I'm going to revert this.

Committed as 395cc55d8966

Committed as 07ede751612f

I think hw->phy.autoneg_wait_to_complete is also set to false in the driver earlier in em_if_attach_pre() and never changes.

I'm less optimistic about the ifp seeing the change required in all drivers, @gallatin odes anyone have any idea on the impact of the ifp pointer chase and fetch versus grabbing it from the already fetched scctx in these hot paths? How would we sync scctx_capenable with ifnet, that seems to be a critical missing implementation from the outset.

In D31485#754650, @erj wrote:

Though, part of the problem with this patch is that I'm not entirely sure with what I want the parameters of ift_txq_select to be.

Right now it takes the mbuf pointer which is enough for what we want to do with it in an upcoming ice(4) patch, but in the future I'll need to use DSCP/IP ToS information in that function as well. I think I should be able to put that information inside the mbuf along with something like an M_DSCP flag. Though, one version I had before also had the if_pkt_info_t struct as well as having the headers (partially) parsed before this queue selection function was called in order to avoid having to modify mbufs like that.

LGTM

LGTM

In D32585#737015, @np wrote:

Note. This change requires PCBGROUP to be retired.

Have you circulated this proposal in the wider -net and -vendor community? I know of one downstream that uses this feature.

It was pointed out to me there is still an issue in that we pick up a new sx within the epoch, both in create and timeout with my patch as is For some reason the epoch _preempt variety doesn't produce a diagnostic so I will look into that code.

Thanks @markj what do you think of this approach using epoch?

In D32388#730851, @jlduran_gmail.com wrote:

Thank you!

Side note:

Also see: D31215

Jose submitted a variant where the driver (e1000 in this case) calls the iflib_led_create() function in its post attach. I am open to either approach if anyone has any thoughts one way or the other.

Ping. Awaiting approval

@gallatin can you have another look? The locking model on the e1000 is tricky due to the HW so I stashed the info on attach. There's a separate KASSERT fix for em_print_nvm_info for review as well.

will look at this once remote access is available

Enable the msix rxtx vec

Committed in https://cgit.freebsd.org/src/commit/?id=450c3f8b3d259c7eb82488319aff45f1f6554aaf

ts=8

I would like to commit this on the 27th if there is no feedback.

Need to fix ixv

I'd like to commit this on the 27th (2 week feedback window) if there is no other discussion.

This may be a timing issue planning to check a few more things

Concept ACK. I don't have anything to test it on at the moment.

@imp For posterity, the way I generated the patches.. @erj maybe the DPDK team can use a method like this too so we stay in lockstep

Update with remaining catch-up diff

Seems fine to me.

@stevek @ian -- @mjg is reporting deadlocks with a heavy userspace load with this change on stable/12 on a 2GB arm platform.

In D31621#713873, @erj wrote:

In D31621#713497, @kbowling wrote:

I'd like to bias for action in any case because this all looks correct and needful so the sooner I can start committing the happier I'll be. I used a diff tool to do the first pass because I didn't realize the DPDK devs were doing incremental updates. In the past the shared code has landed as a big change with little associated revision information. After finding the commits I am leaning toward replaying the commits so we can preserve the why.

I think this is still something that needs to be discussed within the community. We have the capability to do that (see the Linux kernel!), but I already see issues with e.g. authorship from a large amount of commits not being that of a FreeBSD committer, as well as potentially polluting the FreeBSD git logs with lots of Intel Ethernet-only stuff.

In D31621#713498, @imp wrote: