Created attachment 146347 [details] ixgbe.c fix IXGBE_LEGACY_TX The ixgbe driver + PF/ALTQ under stable/9 and likely stable/10 and head is broken. Initially, loading a PF ruleset with ALTQ enabled fails on an ix interface, reporting "ix0: driver does not support altq". This is similar to the behavior over the last few years when dealing with the igb driver. However, I have been using ALTQ + igb with great success by defining IGB_LEGACY_TX in the e1000/igb driver code. I noticed that ixgbe has a similar define IXGBE_LEGACY_TX to enable the legacy, non-multiqueue transmit behavior, that also "enables" ALTQ support. After adding the IXGBE_LEGACY_TX define to ixgbe source, building the driver fails with the following compile errors: /usr/src/sys/dev/ixgbe/ixgbe.c: In function 'ixgbe_msix_que': /usr/src/sys/dev/ixgbe/ixgbe.c:1529: error: invalid type argument of '->' (have 'struct ifaltq') /usr/src/sys/dev/ixgbe/ixgbe.c:1529: error: invalid type argument of '->' (have 'struct ifaltq') /usr/src/sys/dev/ixgbe/ixgbe.c: In function 'ixgbe_local_timer': /usr/src/sys/dev/ixgbe/ixgbe.c:2077: error: 'struct tx_ring' has no member named 'txq_task' /usr/src/sys/dev/ixgbe/ixgbe.c: In function 'ixgbe_free_transmit_buffers': /usr/src/sys/dev/ixgbe/ixgbe.c:3255: error: 'struct tx_ring' has no member named 'br' /usr/src/sys/dev/ixgbe/ixgbe.c:3256: error: 'struct tx_ring' has no member named 'br' So it seems that the IXGBE_LEGACY_TX path no longer compiles successfully, and perhaps never did? Using e1000 as a reference, fixing the pointer error, and looking at previous revisions of ixgbe.c, I was able to come up with the attached patch that got the driver to compile while having IXGBE_LEGACY_TX defined. Note that the following svn diff is against HEAD, which as far as I can tell contains the same broken IXGBE_LEGACY_TX path as stable/9 and stable/10. Using a stable/9 kernel with the attached patch allowed me to load my PF ruleset on a machine with an ixgbe interface and ALTQ enabled. i.e. I no longer received the "driver does not support altq error". Queueing on the ix interface now appears to work as it should. I am hoping someone can help verify my work and perhaps audit and correct the IXGBE_LEGACY_TX path currently in the svn tree. Also, FWIW, here is relevant pciconf output for the ixbge card. ix0@pci0:1:0:0: class=0x020000 card=0x00038086 chip=0x10fb8086 rev=0x01 hdr=0x00 vendor = 'Intel Corporation' device = '82599EB 10-Gigabit SFI/SFP+ Network Connection' class = network subclass = ethernet cap 01[40] = powerspec 3 supports D0 D3 current D0 cap 05[50] = MSI supports 1 message, 64 bit, vector masks cap 11[70] = MSI-X supports 64 messages in map 0x20 enabled cap 10[a0] = PCI-Express 2 endpoint max data 128(512) link x8(x8) ecap 0001[100] = AER 1 0 fatal 0 non-fatal 1 corrected ecap 0003[140] = Serial 1 0023faffff300715 ecap 000e[150] = unknown 1 ecap 0010[160] = unknown 1 ix1@pci0:1:0:1: class=0x020000 card=0x00038086 chip=0x10fb8086 rev=0x01 hdr=0x00 vendor = 'Intel Corporation' device = '82599EB 10-Gigabit SFI/SFP+ Network Connection' class = network subclass = ethernet cap 01[40] = powerspec 3 supports D0 D3 current D0 cap 05[50] = MSI supports 1 message, 64 bit, vector masks cap 11[70] = MSI-X supports 64 messages in map 0x20 enabled cap 10[a0] = PCI-Express 2 endpoint max data 128(512) link x8(x8) ecap 0001[100] = AER 1 0 fatal 0 non-fatal 1 corrected ecap 0003[140] = Serial 1 0023faffff300715 ecap 000e[150] = unknown 1 ecap 0010[160] = unknown 1 Thanks!
Changes were made to IXGBE_LEGACY_TX in this ixgbe version: https://downloadcenter.intel.com/Detail_Desc.aspx?DwnldID=14688 Does using that help? - Eric
Ugh, I totally dropped the ball on this one. I'm sorry. I'll see if I can get releng support to commit this before 10.1-RELEASE occurs. Thanks! -a
From what I can tell, the changes in the newer ixgbe driver here: https://downloadcenter.intel.com/Detail_Desc.aspx?DwnldID=14688 Fix two of three problems with the IXGBE_LEGACY_TX path that I changed in the attached diff. My guess is because they were fixed because its pretty obvious when auditing the code. #1 #ifdef IXGBE_LEGACY_TX - if (!IFQ_DRV_IS_EMPTY(ifp->if_snd)) + if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) This is fixed in the newer Intel driver. Not in FBSD. #2 -#ifdef IXGBE_LEGACY_TX - if (txr->br != NULL) - buf_ring_free(txr->br, M_DEVBUF); -#endif This also seems to be fixed in the Intel driver, not FBSD, which as far as I can tell affects the non-legacy path behavior (i.e., most everyone). In my diff I opted to remove it, but in the newer Intel driver apparently its implied that its necessary for the non-legacy path, as the #ifdef is changed to #ifndef. #3 else if (txr->queue_status == IXGBE_QUEUE_WORKING) +#ifndef IXGBE_LEGACY_TX taskqueue_enqueue(que->tq, &txr->txq_task); +#else + taskqueue_enqueue(que->tq, &que->que_task); +#endif I still believe this change is necessary, and its not in the newer Intel driver from what I can tell, and its not so obvious unless you actually try to use the legacy path. Again, I'm not an expert, so I hope someone can validate all of this. 10/stable (and HEAD, I think) still have all three problems. It would indeed be nice for this to be fixed in 10.1-RELEASE. Thanks!
Why do you believe the third change is necessary? Is there a reason the extra code in the que_task tasklet must run in the legacy tx case?
(In reply to Eric Joyner from comment #4) > Why do you believe the third change is necessary? Is there a reason the > extra code in the que_task tasklet must run in the legacy tx case? Because of this part of the compile error when I tried to build a new kernel. /usr/src/sys/dev/ixgbe/ixgbe.c: In function 'ixgbe_local_timer': /usr/src/sys/dev/ixgbe/ixgbe.c:2077: error: 'struct tx_ring' has no member named 'txq_task' Everything else relying on txr->txq_task is either confined to within a multiqueue (non-legacy) function or there is an #ifndef IXGBE_LEGACY_TX around it. Furthermore, if you look at an older version of the ixgbe_local_timer function, it has taskqueue_enqueue(que->tq, &que->que_task) instead of taskqueue_enqueue(que->tq, &txr->txq_task); Here is the change where that happened. http://svnweb.freebsd.org/base/head/sys/dev/ixgbe/ixgbe.c?annotate=271648 Line 2066 http://svnweb.freebsd.org/base?view=revision&revision=251964 Note that was the only line in ixgbe_local_timer that was changed. I believe this change was made without consideration of the LEGACY_TX path, which is strange, because the commit was evidently intended to add ALTQ support via the LEGACY_TX path. Also, the igb/e1000 driver (if_igb.c) has a similar behavior in the same function, where taskqueue_enqueue(que->tq, &que->que_task) is used.
You didn't try a kernel build with the newer driver that I posted?
(In reply to Eric Joyner from comment #6) > You didn't try a kernel build with the newer driver that I posted? I have not.
(In reply to ncrogers from comment #7) > (In reply to Eric Joyner from comment #6) > > You didn't try a kernel build with the newer driver that I posted? > > I have not. I finally had a chance to try this. The newer driver you posted (2.5.25) seems to compile successfully with IXGBE_LEGACY_TX defined. This happens to be under 9-STABLE. I was not able to test ALTQ behavior with the new driver, but I imagine it works since the LEGACY path is fixed.
Any chance we can get some kind of resolution on this before 10.1? It seems like simply upgrading the ixgbe driver with the latest from Intel fixes the problem. If that is not possible, the changes in the patch I posted are still working for me in production
On 10.1-RC1, with the change below, I still get the dreaded "pfctl: igb1: driver does not support altq". If ixgbe is targeted for 10.1-RELEASE, can igb be fixed as well? Index: sys/modules/igb/Makefile =================================================================== --- sys/modules/igb/Makefile (revision 272488) +++ sys/modules/igb/Makefile (working copy) @@ -21,7 +21,7 @@ # instead use the older if_start non-multiqueue capable interface. # This might be desireable for testing, or to enable the use of # ALTQ. -#CFLAGS += -DIGB_LEGACY_TX +CFLAGS += -DIGB_LEGACY_TX .if !defined(KERNBUILDDIR) .if ${MK_INET_SUPPORT} != "no" I posted more about it here: https://forums.freebsd.org/viewtopic.php?f=44&t=48283
(In reply to Dan from comment #10) > On 10.1-RC1, with the change below, I still get the dreaded "pfctl: igb1: > driver does not support altq". If ixgbe is targeted for 10.1-RELEASE, can > igb be fixed as well? > > Index: sys/modules/igb/Makefile > =================================================================== > --- sys/modules/igb/Makefile (revision 272488) > +++ sys/modules/igb/Makefile (working copy) > @@ -21,7 +21,7 @@ > # instead use the older if_start non-multiqueue capable interface. > # This might be desireable for testing, or to enable the use of > # ALTQ. > -#CFLAGS += -DIGB_LEGACY_TX > +CFLAGS += -DIGB_LEGACY_TX > > .if !defined(KERNBUILDDIR) > .if ${MK_INET_SUPPORT} != "no" > > I posted more about it here: > > https://forums.freebsd.org/viewtopic.php?f=44&t=48283 I've always had to add #define IXGBE_LEGACY_TX to sys/dev/e1000/if_igb.c and sys/dev/e1000/if_igb.h to get it to work. Uncommenting CFLAGS in the igb Makefile doesn't work when compiling a kernel. I believe it needs to be added as a kernel option of some kind to be cleaner.
(In reply to ncrogers from comment #11) > (In reply to Dan from comment #10) > > On 10.1-RC1, with the change below, I still get the dreaded "pfctl: igb1: > > driver does not support altq". If ixgbe is targeted for 10.1-RELEASE, can > > igb be fixed as well? > > > > Index: sys/modules/igb/Makefile > > =================================================================== > > --- sys/modules/igb/Makefile (revision 272488) > > +++ sys/modules/igb/Makefile (working copy) > > @@ -21,7 +21,7 @@ > > # instead use the older if_start non-multiqueue capable interface. > > # This might be desireable for testing, or to enable the use of > > # ALTQ. > > -#CFLAGS += -DIGB_LEGACY_TX > > +CFLAGS += -DIGB_LEGACY_TX > > > > .if !defined(KERNBUILDDIR) > > .if ${MK_INET_SUPPORT} != "no" > > > > I posted more about it here: > > > > https://forums.freebsd.org/viewtopic.php?f=44&t=48283 > > I've always had to add #define IXGBE_LEGACY_TX to sys/dev/e1000/if_igb.c and > sys/dev/e1000/if_igb.h to get it to work. Uncommenting CFLAGS in the igb > Makefile doesn't work when compiling a kernel. I believe it needs to be > added as a kernel option of some kind to be cleaner. I guess in the last 3 years, no one has bother to do it because they keep secretly hoping we can stop using legacy mode to use ALTQ :) Pretty sure it would be easy to add as a kernel option. I even saw a thread where someone described what it would take. I wonder why it never got committed.
Thanks, btw. That worked.
I think a patch like the following would make using legacy mode easier. This one is for IGB, but a similar one would work for IXGBE: Index: sys/conf/options =================================================================== --- sys/conf/options (revision 272659) +++ sys/conf/options (working copy) @@ -405,6 +405,7 @@ ETHER_8023 opt_ef.h ETHER_II opt_ef.h ETHER_SNAP opt_ef.h +IGB_LEGACY_TX opt_igb.h INET opt_inet.h INET6 opt_inet6.h IPDIVERT Index: sys/dev/e1000/if_igb.c =================================================================== --- sys/dev/e1000/if_igb.c (revision 272659) +++ sys/dev/e1000/if_igb.c (working copy) @@ -33,6 +33,8 @@ /*$FreeBSD$*/ +#include "opt_igb.h" + #include "opt_inet.h" #include "opt_inet6.h" Index: sys/dev/e1000/if_igb.h =================================================================== --- sys/dev/e1000/if_igb.h (revision 272659) +++ sys/dev/e1000/if_igb.h (working copy) @@ -35,6 +35,8 @@ #ifndef _IGB_H_DEFINED_ #define _IGB_H_DEFINED_ +#include "opt_igb.h" + /* Tunables */ /*
(In reply to Dan from comment #14) > I think a patch like the following would make using legacy mode easier. > This one is for IGB, but a similar one would work for IXGBE: I think thats a good idea and something that I've requested in the past. I am not sure if there is an existing PR for it or not. Can you please start a new PR/bug in regards to making IGB/IXGBE LEGACY a kernel option? I would like to keep this bug focussed on fixing the IXGBE_LEGACY_TX path which currently doesn't even compile. Thanks. > > Index: sys/conf/options > =================================================================== > --- sys/conf/options (revision 272659) > +++ sys/conf/options (working copy) > @@ -405,6 +405,7 @@ > ETHER_8023 opt_ef.h > ETHER_II opt_ef.h > ETHER_SNAP opt_ef.h > +IGB_LEGACY_TX opt_igb.h > INET opt_inet.h > INET6 opt_inet6.h > IPDIVERT > Index: sys/dev/e1000/if_igb.c > =================================================================== > --- sys/dev/e1000/if_igb.c (revision 272659) > +++ sys/dev/e1000/if_igb.c (working copy) > @@ -33,6 +33,8 @@ > /*$FreeBSD$*/ > > > +#include "opt_igb.h" > + > #include "opt_inet.h" > #include "opt_inet6.h" > > Index: sys/dev/e1000/if_igb.h > =================================================================== > --- sys/dev/e1000/if_igb.h (revision 272659) > +++ sys/dev/e1000/if_igb.h (working copy) > @@ -35,6 +35,8 @@ > #ifndef _IGB_H_DEFINED_ > #define _IGB_H_DEFINED_ > > +#include "opt_igb.h" > + > /* Tunables */ > > /*
I submitted the following bug. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=194197 I added code to enable IXGBE as well; hopefully it was correct.
Closing, there's no mention of IXGBE_LEGACY_TX and in the linked bug, Kevin mentions that it should be fixed for 12 and newer.