In D5211#112003, @gallatin wrote:

I don't see a change to sys/conf/files to add iflib.c

Similarly, I don't see changes in the ixgbe / ixl diffs to add a depend on iflib in sys/conf/files.*

Wasn't in the diff itself - see update.

When linking, the mp_ring here conflicts with the one in the cxgbe driver if both iflib and cxgbe are included in the kernel.

In D5211#112784, @gallatin wrote:

When linking, the mp_ring here conflicts with the one in the cxgbe driver if both iflib and cxgbe are included in the kernel.

Will rename.

Build comments

To follow up on my last comment, fixing this (patch appended) seems to result in the kernel actually sending down packets with csum offload, which then causes the ixl NIC to stop passing traffic, and to die occasionally with: "ixl0: Malicious Driver Detection event 0x02 on TX queue 5 pf number 0x00"

diff --git a/sys/net/iflib.c b/sys/net/iflib.c
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -1681,6 +1681,7 @@ static void
 iflib_init_locked(if_ctx_t ctx)
 {
        if_softc_ctx_t sctx = &ctx->ifc_softc_ctx;
+       if_t ifp = ctx->ifc_ifp;
        iflib_fl_t fl;
        iflib_txq_t txq;
        iflib_rxq_t rxq;
@@ -1697,6 +1698,15 @@ iflib_init_locked(if_ctx_t ctx)
                iflib_netmap_rxq_init(ctx, rxq);
        }
 
+
+       /* Set hardware offload abilities */
+       if_clearhwassist(ifp);
+       if (if_getcapenable(ifp) & IFCAP_TXCSUM)
+               if_sethwassistbits(ifp, CSUM_TCP | CSUM_UDP, 0);
+       if (if_getcapenable(ifp) & IFCAP_TSO4)
+               if_sethwassistbits(ifp, CSUM_TSO, 0);
+       if (if_getcapenable(ifp) & IFCAP_TXCSUM_IPV6)
+               if_sethwassistbits(ifp,  (CSUM_TCP_IPV6 | CSUM_UDP_IPV6), 0);
        IFDI_INIT(ctx);
        for (i = 0, rxq = ctx->ifc_rxqs; i < sctx->isc_nqsets; i++, rxq++) {
                for (j = 0, fl = rxq->ifr_fl; j < rxq->ifr_nfl; j++, fl++) {

We still seem to be missing the device_register api required by iflib. Eg:

diff --git a/sys/kern/device_if.m b/sys/kern/device_if.m
index a5319c6..18d9fd7 100644
--- a/sys/kern/device_if.m
+++ b/sys/kern/device_if.m
@@ -62,6 +62,11 @@ CODE {
        {
            return 0;
        }
+
+       static void *null_register(device_t dev)
+       {
+           return NULL;
+       }
 };
        
 /**
@@ -316,3 +321,7 @@ METHOD int resume {
 METHOD int quiesce {
        device_t dev;
 } DEFAULT null_quiesce;
+
+METHOD void * register {
+       device_t dev;
+} DEFAULT null_register;

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

ixl0: using 28 queues
ixl0: Using MSIX interrupts with 29 vectors
ixl0: allocated for 28 queues

Fatal trap 12: page fault while in kernel mode
cpuid = 18; apic id = 14
fault virtual address = 0x1818
fault code = supervisor write data, page not present
instruction pointer = 0x20:0xffffffff80ad711c
stack pointer = 0x28:0xfffffe3fca9f3fb0
frame pointer = 0x28:0xfffffe3fca9f4020
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 = 123 (kldload)
[ thread pid 123 tid 100220 ]
Stopped at taskqgroup_attach+0x4c: lock cmpxchgq %rsi,0x1818(%r15)
db> bt
Tracing pid 123 tid 100220 td 0xfffff8013b123000
taskqgroup_attach() at taskqgroup_attach+0x4c/frame 0xfffffe3fca9f4020
iflib_irq_alloc_generic() at iflib_irq_alloc_generic+0x357/frame 0xfffffe3fca9f40b0
ixl_if_msix_intr_assign() at ixl_if_msix_intr_assign+0x66/frame 0xfffffe3fca9f4130iflib_device_register() at iflib_device_register+0x17a9/frame 0xfffffe3fca9f4470
iflib_device_attach() at iflib_device_attach+0xb7/frame 0xfffffe3fca9f44a0
device_attach() at device_attach+0x423/frame 0xfffffe3fca9f4500
pci_driver_added() at pci_driver_added+0xed/frame 0xfffffe3fca9f4540
devclass_driver_added() at devclass_driver_added+0x7d/frame 0xfffffe3fca9f4580
devclass_add_driver() at devclass_add_driver+0x11c/frame 0xfffffe3fca9f45c0
module_register_init() at module_register_init+0x177/frame 0xfffffe3fca9f45f0
linker_load_module() at linker_load_module+0xca8/frame 0xfffffe3fca9f4920
kern_kldload() at kern_kldload+0xea/frame 0xfffffe3fca9f4960
sys_kldload() at sys_kldload+0x5b/frame 0xfffffe3fca9f4990
amd64_syscall() at amd64_syscall+0x502/frame 0xfffffe3fca9f4ab0
Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe3fca9f4ab0

• syscall (304, FreeBSD ELF64, sys_kldload), rip = 0x80086b83a, rsp = 0x7fffffffe7e8, rbp = 0x7fffffffed00 ---

db>

In D5211#114950, @gallatin wrote:

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

D5205 is supposed to fix this crash.

In D5211#114958, @erj wrote:

In D5211#114950, @gallatin wrote:

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

D5205 is supposed to fix this crash.

That pre-dates iflib; iflib (mostly) obviates that review

In D5211#114950, @gallatin wrote:

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

I can confirm that moving back to SI_SUB_SMP in the DECLARE_MODULE macro fixes it enough to at least work with an ixl module loaded post-boot.

diff --git a/sys/net/iflib.c b/sys/net/iflib.c
index ab5f74c..37d970e 100644
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -421,7 +421,7 @@ static moduledata_t iflib_moduledata = {
        NULL
 };
 
-DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_INIT_IF, SI_ORDER_ANY);
+DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_SMP, SI_ORDER_ANY);
 MODULE_VERSION(iflib, 1);
 
 MODULE_DEPEND(iflib, pci, 1, 1, 1);

In D5211#114944, @gallatin wrote:

We still seem to be missing the device_register api required by iflib. Eg:

Sorry, I'm sure it was in an earlier patch. Will put it back up.

In D5211#114961, @gallatin wrote:

In D5211#114950, @gallatin wrote:

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

I can confirm that moving back to SI_SUB_SMP in the DECLARE_MODULE macro fixes it enough to at least work with an ixl module loaded post-boot.

diff --git a/sys/net/iflib.c b/sys/net/iflib.c
index ab5f74c..37d970e 100644
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -421,7 +421,7 @@ static moduledata_t iflib_moduledata = {
        NULL
 };
 
-DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_INIT_IF, SI_ORDER_ANY);
+DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_SMP, SI_ORDER_ANY);
 MODULE_VERSION(iflib, 1);
 
 MODULE_DEPEND(iflib, pci, 1, 1, 1);

Ok. I needed that change to make my system boot with ixgbe linked in to the kernel.

In D5211#114387, @gallatin wrote:

To follow up on my last comment, fixing this (patch appended) seems to result in the kernel actually sending down packets with csum offload, which then causes the ixl NIC to stop passing traffic, and to die occasionally with: "ixl0: Malicious Driver Detection event 0x02 on TX queue 5 pf number 0x00"

The flag setting appears to have gotten lost in a mismerge jumping between branches. The first place I'd look to fix that error would be at my changes to ixl when I removed packet parsing.

In D5211#114961, @gallatin wrote:

In D5211#114950, @gallatin wrote:

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

I can confirm that moving back to SI_SUB_SMP in the DECLARE_MODULE macro fixes it enough to at least work with an ixl module loaded post-boot.

diff --git a/sys/net/iflib.c b/sys/net/iflib.c
index ab5f74c..37d970e 100644
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -421,7 +421,7 @@ static moduledata_t iflib_moduledata = {
        NULL
 };
 
-DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_INIT_IF, SI_ORDER_ANY);
+DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_SMP, SI_ORDER_ANY);
 MODULE_VERSION(iflib, 1);
 
 MODULE_DEPEND(iflib, pci, 1, 1, 1);

Uhm ... did you pull in the corresponding update to the task queue patch? That's probably the real problem.

In D5211#114963, @kmacy wrote:

In D5211#114961, @gallatin wrote:

In D5211#114950, @gallatin wrote:

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

I can confirm that moving back to SI_SUB_SMP in the DECLARE_MODULE macro fixes it enough to at least work with an ixl module loaded post-boot.

diff --git a/sys/net/iflib.c b/sys/net/iflib.c
index ab5f74c..37d970e 100644
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -421,7 +421,7 @@ static moduledata_t iflib_moduledata = {
        NULL
 };
 
-DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_INIT_IF, SI_ORDER_ANY);
+DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_SMP, SI_ORDER_ANY);
 MODULE_VERSION(iflib, 1);
 
 MODULE_DEPEND(iflib, pci, 1, 1, 1);

Uhm ... did you pull in the corresponding update to the task queue patch? That's probably the real problem.

Yes.

I think the problem is actually in that file. Specifically, the SYSINIT() in the TASKQGROUP_DEFINE() macro that uses SI_SUB_SMP pretty much makes certain that when the DRIVER_MODULE executes earlier, the taskq stuff will still be NULL. Then that stays cached, and the drivers trip over it.

In D5211#114964, @gallatin wrote:

In D5211#114963, @kmacy wrote:

In D5211#114961, @gallatin wrote:

In D5211#114950, @gallatin wrote:

Hmm.. Even when loading the module at boot on -current, I'm still seeing a panic:

I can confirm that moving back to SI_SUB_SMP in the DECLARE_MODULE macro fixes it enough to at least work with an ixl module loaded post-boot.

diff --git a/sys/net/iflib.c b/sys/net/iflib.c
index ab5f74c..37d970e 100644
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -421,7 +421,7 @@ static moduledata_t iflib_moduledata = {
        NULL
 };
 
-DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_INIT_IF, SI_ORDER_ANY);
+DECLARE_MODULE(iflib, iflib_moduledata, SI_SUB_SMP, SI_ORDER_ANY);
 MODULE_VERSION(iflib, 1);
 
 MODULE_DEPEND(iflib, pci, 1, 1, 1);

Uhm ... did you pull in the corresponding update to the task queue patch? That's probably the real problem.

Yes.

I think the problem is actually in that file. Specifically, the SYSINIT() in the TASKQGROUP_DEFINE() macro that uses SI_SUB_SMP pretty much makes certain that when the DRIVER_MODULE executes earlier, the taskq stuff will still be NULL. Then that stays cached, and the drivers trip over it.

No. The updated taskq patch doesn't use SI_SUB_SMP.

In D5211#114965, @kmacy wrote:

No. The updated taskq patch doesn't use SI_SUB_SMP.

There are 2 patches in that review. The second patch updates 2 SYSINITS to use the IF order, but leaves one at SMP.

In D5211#114966, @gallatin wrote:

In D5211#114965, @kmacy wrote:

No. The updated taskq patch doesn't use SI_SUB_SMP.

There are 2 patches in that review. The second patch updates 2 SYSINITS to use the IF order, but leaves one at SMP.

Right. Adjust updates the number of threads once SMP comes on line. Are you saying that that's still a problem?

It's odd that ixgbe works with the change. Although I admit I haven't tried ixgbe as a module with that change.

Right. Adjust updates the number of threads once SMP comes on line. Are you saying that that's still a problem?

Honestly, I haven't thought much about it. I'm just trying to get things to not crash so that I can try to get ixl working in a controlled environment before I test it for real. All I can say for sure is that the patch, as it stands today, goes "kaboom" with both ixl compiled as a module, and with it compiled into the kernel.

I'll try to diagnose this after I get ixl not to barf on the descriptors we're sending down to it when csum offload is enabled.

Are the changes to if_an, if_bxe, and kern_mbuf.c intentional?

The change to kern_mbuf allows iflib to avoid gratuitous memory touches on free. The other driver changes are_not_ optional as they fix use after free memory corruptions in those drivers' probe routines that have hit iflib on driver reload (unload and then reload a driver kmod).

You can leave out the mbuf changes in the initial commit I guess. They also rerequire adding defines to mbuf.h which I missed.

I've made the changes to if_an and committed that part of the patch to freebsd-head

I need the changes to mbuf.h

Sorry. Thought you'd just remove references to that flag.

That's not true. device_set_desc and friends are specifically designed to be operated on in device_probe.

Maybe at one point it worked. It causes use after frees now.

In any event. I don't see any point in setting the description until attach. It may be the freeing of descbuf isn't permitted, but in that case you'd have a leak if it didn't subsequently attach. This is not a robust interface.

There is a directive for platforms without 64-bit atomics. Such arches
should put that in their param.h, sorry for the oversight. A lot of balls
in the air.

-M

Thank you for quick reply!

Another issue is:
src/sys/net/iflib.c: In function '_iflib_fl_refill':
src/sys/net/iflib.c:1538: warning: comparison is always true due to limited range of data type
src/sys/net/iflib.c: In function 'iflib_fl_bufs_free':
src/sys/net/iflib.c:1667: warning: comparison is always true due to limited range of data type

I suppose it's due to unsigned types. But build is failed due to warnings.

Thanks!

Sorry. Send Scott a patch. I can't do anything about it.

-M

Thanks! BTW, it was last compilation issue under MIPS o32.