Hi Nick,
Thanks for the testing, I am glad I could help. Please note that by setting:
static int igb_num_queues = 1;
You are effectively only using 1 TX queue from the hardware (instead of
4 or 8) so this might not be applicable to a generic kernel without ALTQ.
Best regards,
Karim.
On 02/04/2013 1:17 PM, Nick Rogers wrote:
On Tue, Apr 2, 2013 at 9:17 AM, Nick Rogers <ncrog...@gmail.com> wrote:
On Tue, Apr 2, 2013 at 7:47 AM, Karim Fodil-Lemelin
<fodillemlinka...@gmail.com> wrote:
Hi Nick,
Unfortunately I do not have a FBSD 9 box around where I can compile and test
this so bear with me as this is pretty much straight out of looking at the
source code directly (i.e it might not even compile).
But if your desperate please try the following (untested) patch (applied to
stable/9):
Thanks for the attempt. The patch does not apply cleanly to stable/9
for me. Also I am trying to work with the latest commits Jack has made
to HEAD that allow defining IGB_LEGACY_TX to disable the new
multiqueue stack. It seems the gist of this is the same as what you
have changed, unless I am missing something.
OK, although your patch did not compile even after applying it
manually, It made me understand the gist of what you were saying about
"keeping igb_start() defined and using igb_mq_start_locked() inside it
instead of igb_start_locked()". I was able to make my own patch that
is very similar to your intention (and your patch) and it compiled,
and solved my problem. ALTQ now works with igb (I am able to load my
pf.conf, use PF, etc). Thanks a lot for the help. I will try and work
with Jack to perhaps integrate this in a more official manner, as this
is indeed a bit different than what he has changed in HEAD w.r.t
IGB_LEGACY_TX configurability.
Below is the diff of my changes against stable/9.
Index: sys/dev/e1000/if_igb.c
===================================================================
--- sys/dev/e1000/if_igb.c (revision 249024)
+++ sys/dev/e1000/if_igb.c (working copy)
@@ -179,13 +179,13 @@
static int igb_shutdown(device_t);
static int igb_suspend(device_t);
static int igb_resume(device_t);
+static void igb_start(struct ifnet *);
#if __FreeBSD_version >= 800000
static int igb_mq_start(struct ifnet *, struct mbuf *);
static int igb_mq_start_locked(struct ifnet *, struct tx_ring *);
static void igb_qflush(struct ifnet *);
static void igb_deferred_mq_start(void *, int);
#else
-static void igb_start(struct ifnet *);
static void igb_start_locked(struct tx_ring *, struct ifnet *ifp);
#endif
static int igb_ioctl(struct ifnet *, u_long, caddr_t);
@@ -377,7 +377,7 @@
** This will autoconfigure based on
** the number of CPUs if left at 0.
*/
-static int igb_num_queues = 0;
+static int igb_num_queues = 1;
TUNABLE_INT("hw.igb.num_queues", &igb_num_queues);
SYSCTL_INT(_hw_igb, OID_AUTO, num_queues, CTLFLAG_RDTUN, &igb_num_queues, 0,
"Number of queues to configure, 0 indicates autoconfigure");
@@ -926,6 +926,8 @@
txr->queue_status |= IGB_QUEUE_WORKING;
}
}
+
+#else /* __FreeBSD_version >= 800000 */
/*
* Legacy TX driver routine, called from the
@@ -940,13 +942,16 @@
if (ifp->if_drv_flags & IFF_DRV_RUNNING) {
IGB_TX_LOCK(txr);
- igb_start_locked(txr, ifp);
+ #if __FreeBSD_version < 800000
+ igb_start_locked(txr, ifp);
+ #else
+ igb_mq_start_locked(ifp, txr);
+ #endif
IGB_TX_UNLOCK(txr);
}
return;
}
-#else /* __FreeBSD_version >= 800000 */
/*
** Multiqueue Transmit Entry:
@@ -3089,12 +3094,11 @@
#if __FreeBSD_version >= 800000
ifp->if_transmit = igb_mq_start;
ifp->if_qflush = igb_qflush;
-#else
+#endif
ifp->if_start = igb_start;
IFQ_SET_MAXLEN(&ifp->if_snd, adapter->num_tx_desc - 1);
ifp->if_snd.ifq_drv_maxlen = adapter->num_tx_desc - 1;
IFQ_SET_READY(&ifp->if_snd);
-#endif
ether_ifattach(ifp, adapter->hw.mac.addr);
diff --git a/sys/dev/e1000/if_igb.c b/sys/dev/e1000/if_igb.c
index 30bb052..3a6de2e 100644
--- a/sys/dev/e1000/if_igb.c
+++ b/sys/dev/e1000/if_igb.c
@@ -179,13 +179,13 @@ static int igb_detach(device_t);
static int igb_shutdown(device_t);
static int igb_suspend(device_t);
static int igb_resume(device_t);
+static void igb_start(struct ifnet *);
#if __FreeBSD_version >= 800000
static int igb_mq_start(struct ifnet *, struct mbuf *);
static int igb_mq_start_locked(struct ifnet *, struct tx_ring *);
static void igb_qflush(struct ifnet *);
static void igb_deferred_mq_start(void *, int);
#else
-static void igb_start(struct ifnet *);
static void igb_start_locked(struct tx_ring *, struct ifnet *ifp);
#endif
static int igb_ioctl(struct ifnet *, u_long, caddr_t);
@@ -377,7 +377,7 @@ SYSCTL_INT(_hw_igb, OID_AUTO, header_split,
CTLFLAG_RDTUN, &igb_header_split, 0,
** This will autoconfigure based on
** the number of CPUs if left at 0.
*/
-static int igb_num_queues = 0;
+static int igb_num_queues = 1;
TUNABLE_INT("hw.igb.num_queues", &igb_num_queues);
SYSCTL_INT(_hw_igb, OID_AUTO, num_queues, CTLFLAG_RDTUN, &igb_num_queues,
0,
"Number of queues to configure, 0 indicates autoconfigure");
@@ -926,6 +926,8 @@ igb_start_locked(struct tx_ring *txr, struct ifnet *ifp)
txr->queue_status |= IGB_QUEUE_WORKING;
}
}
+
+#endif /* __FreeBSD_version < 800000 */
/*
* Legacy TX driver routine, called from the
@@ -940,14 +942,17 @@ igb_start(struct ifnet *ifp)
if (ifp->if_drv_flags & IFF_DRV_RUNNING) {
IGB_TX_LOCK(txr);
+#if __FreeBSD_version < 800000
+ igb_start_locked(txr, ifp);
+#else
+ igb_mq_start_locked(ifp, txr, NULL);
+#endif
igb_start_locked(txr, ifp);
IGB_TX_UNLOCK(txr);
}
return;
}
-#else /* __FreeBSD_version >= 800000 */
-
/*
** Multiqueue Transmit Entry:
** quick turnaround to the stack
@@ -3089,12 +3094,11 @@ igb_setup_interface(device_t dev, struct adapter
*adapter)
#if __FreeBSD_version >= 800000
ifp->if_transmit = igb_mq_start;
ifp->if_qflush = igb_qflush;
-#else
+#endif
ifp->if_start = igb_start;
IFQ_SET_MAXLEN(&ifp->if_snd, adapter->num_tx_desc - 1);
ifp->if_snd.ifq_drv_maxlen = adapter->num_tx_desc - 1;
IFQ_SET_READY(&ifp->if_snd);
-#endif
ether_ifattach(ifp, adapter->hw.mac.addr);
On 02/04/2013 9:58 AM, Nick Rogers wrote:
On Tuesday, April 2, 2013, Karim Fodil-Lemelin wrote:
Hi Nick,
Can you verify that you have at least one of those options in your kernel
config file:
ALTQ_CBQ
ALTQ_PRIQ
ALTQ_HFSC
Yes I have hfsc included in the kernel. I have other machines using altq
with em(4) interfaces and similar pf configurations on the same kernel
that
work fine.
Regards,
Karim.
On 01/04/2013 8:22 PM, Nick Rogers wrote:
On Mon, Apr 1, 2013 at 8:44 AM, Karim Fodil-Lemelin
<fodillemlinka...@gmail.com> wrote:
Hi Jack,
I think this would help M. Rogers case as we have done something similar
here to circumvent the issue and it seems to work well. I would also add
that when using ALTQ we found it much more stable to set the number of
queues to 1:
static int igb_num_queues = 1;
Our approach consisted in keeping igb_start() defined and using
igb_mq_start_locked() inside it instead of igb_start_locked().
Regards,
Karim.
Thanks for the pointer.
I've been working with Jack to get a fix for this in that downgrades
the stack to the older if_start/non-multiqueue interface. See the
following two commits he made to HEAD.
http://svnweb.freebsd.org/**base/head/sys/dev/e1000/if_**
igb.c?revision=248906&view=**markup<http://svnweb.freebsd.org/base/head/sys/dev/e1000/if_igb.c?revision=248906&view=markup>
http://svnweb.freebsd.org/**base/head/sys/dev/e1000/if_**
igb.h?revision=248908&view=**markup<http://svnweb.freebsd.org/base/head/sys/dev/e1000/if_igb.h?revision=248908&view=markup>
I've applied these changes to latest 9.1-STABLE src and included the
IGB_LEGACY_TX in the e1000 Makefile. So far I am not having any luck
getting pfctl to think igb is supported.
i.e. I am still getting the following when loading my PF config:
pfctl -f /etc/pf.conf
pfctl: igb0: driver does not support altq
Can anyone comment on if the above referenced changes that Jack made
should be sufficient to get ALTQ working with igb?
The error is coming from src/contrib/pf/pfctl/pfctl.c. There seems to
be a function that checks if the driver is supported or not but I
cannot figure out why the ioctl is not returning a device.
Here is the device check code for reference:
int
pfctl_add_altq(struct pfctl *pf, struct pf_altq *a)
{
if (altqsupport &&
(loadopt & PFCTL_FLAG_ALTQ) != 0) {
memcpy(&pf->paltq->altq, a, sizeof(struct pf_altq));
if ((pf->opts & PF_OPT_NOACTION) == 0) {
if (ioctl(pf->dev, DIOCADDALTQ, pf->paltq)) {
if (errno == ENXIO)
errx(1, "qtype not
configured");
else if (errno == ENODEV)
errx(1, "%s: driver does not
support "
"altq", a->ifname);
else
err(1, "DIOCADDALTQ");
}
}
pfaltq_store(&pf->paltq->altq)**;
}
return (0);
}
On 28/03/2013 7:16 PM, Jack Vogel wrote:
Have been kept fairly busy with other matters, one thing I could do short
term is
change the defines in igb the way I did in the em driver so you could
still
define
the older if_start entry. Right now those are based on OS version and so
you will
automatically get if_transmit, but I could change it to be IGB_LEGACY_TX
or
so,
and that could be defined in the Makefile.
Would this help?
Jack
On Thu, Mar 28, 2013 at 2:31 PM, Nick Rogers <ncrog...@gmail.com> wrote:
On Tue, Dec 11, 2012 at 1:09 AM, Jack Vogel <jfvo...@gmail.com> wrote:
On Mon, Dec 10, 2012 at 11:58 PM, Gleb Smirnoff <gleb...@freebsd.org>
wrote:
On Mon, Dec 10, 2012 at 03:31:19PM -0800, Jack Vogel wrote:
J> UH, maybe asking the owner of the driver would help :)
J>
J> ... and no, I've never been aware of doing anything to stop
supporting
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
