Bug 193053

Summary: ixgbe(4) IXGBE_LEGACY_TX + ALTQ path broken
Product: Base System Reporter: ncrogers
Component: kernAssignee: freebsd-net (Nobody) <net>
Status: Closed FIXED    
Severity: Affects Some People CC: adrian, dan_256, garga, jfv, ncrogers, pkubaj, ricera10, sbruno
Priority: Normal Keywords: IntelNetworking
Version: 10.1-STABLE   
Hardware: amd64   
OS: Any   
Attachments:
Description Flags
ixgbe.c fix IXGBE_LEGACY_TX none

Description ncrogers 2014-08-26 23:13:11 UTC
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!
Comment 1 Eric Joyner 2014-09-17 23:43:53 UTC
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
Comment 2 Adrian Chadd freebsd_committer freebsd_triage 2014-09-18 03:42:09 UTC
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
Comment 3 ncrogers 2014-09-18 04:11:45 UTC
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!
Comment 4 Eric Joyner 2014-09-18 22:24:50 UTC
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?
Comment 5 ncrogers 2014-09-18 22:53:41 UTC
(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.
Comment 6 Eric Joyner 2014-09-18 23:36:21 UTC
You didn't try a kernel build with the newer driver that I posted?
Comment 7 ncrogers 2014-09-19 00:06:01 UTC
(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.
Comment 8 ncrogers 2014-09-22 23:40:41 UTC
(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.
Comment 9 ncrogers 2014-10-01 18:28:12 UTC
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
Comment 10 Dan 2014-10-04 00:55:42 UTC
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
Comment 11 ncrogers 2014-10-04 01:56:16 UTC
(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.
Comment 12 Dan 2014-10-04 05:02:08 UTC
(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.
Comment 13 Dan 2014-10-04 05:24:40 UTC
Thanks, btw.  That worked.
Comment 14 Dan 2014-10-06 17:52:01 UTC
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 */

 /*
Comment 15 ncrogers 2014-10-06 17:56:59 UTC
(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 */
> 
>  /*
Comment 16 Dan 2014-10-06 18:17:25 UTC
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.
Comment 17 Piotr Kubaj freebsd_committer freebsd_triage 2023-02-03 13:25:01 UTC
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.