Date: Tue, 2 Apr 2013 10:17:11 -0700 From: Nick Rogers <ncrogers@gmail.com> To: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, "jfvogel@gmail.com" <jfvogel@gmail.com> Subject: Re: igb and ALTQ in 9.1-rc3 Message-ID: <CAKOb=Yan9b2LS_WbsMwQEkkN0%2BWRpx8eC1x0arVrVYYC1gXngQ@mail.gmail.com> In-Reply-To: <CAKOb=YbopNZ0yRovKOhsDcwYEX-d0RpyS%2BxGoWQAZo2gU3irAg@mail.gmail.com> References: <1355177348.71087.YahooMailClassic@web121601.mail.ne1.yahoo.com> <50C6656E.3020601@gmail.com> <CAFOYbc=79HwkHm6PAyRba%2Bue_7Xq0dUJvLk48RwqUN0Oz6%2BVFw@mail.gmail.com> <20121211075853.GU48639@FreeBSD.org> <CAFOYbc=Yh3WiUDstjSm-AUz4WMF9VteiydcEb5opnJ_gT4F7ag@mail.gmail.com> <CAKOb=Yb7NMm26=u%2B-0ywMk543WdXc5uiO2omY4=TXpRxPrOWHg@mail.gmail.com> <CAFOYbcn3HpRaaoSPWF5OL6QJCec-Zb2%2BNySqjgqyOXHRaXD26Q@mail.gmail.com> <5159AB72.1050202@gmail.com> <CAKOb=YbDW2kMhnAPUetsGeNdtiaOCOmQ2X-9GXW18wwSUZ8j-A@mail.gmail.com> <515ADACD.8010108@gmail.com> <CAKOb=Ybh=_w5ZSB%2B64bxpzBanheJvMhwd9Pus7sJet9ihn5jSA@mail.gmail.com> <515AEF67.4070206@gmail.com> <CAKOb=YbopNZ0yRovKOhsDcwYEX-d0RpyS%2BxGoWQAZo2gU3irAg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Apr 2, 2013 at 9:17 AM, Nick Rogers <ncrogers@gmail.com> wrote:
> On Tue, Apr 2, 2013 at 7:47 AM, Karim Fodil-Lemelin
> <fodillemlinkarim@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
>>>> <fodillemlinkarim@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 <ncrogers@gmail.com> wrote:
>>>>
>>>> On Tue, Dec 11, 2012 at 1:09 AM, Jack Vogel <jfvogel@gmail.com> wrote:
>>>>
>>>> On Mon, Dec 10, 2012 at 11:58 PM, Gleb Smirnoff <glebius@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-unsubscribe@freebsd.org"
>>
>>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAKOb=Yan9b2LS_WbsMwQEkkN0%2BWRpx8eC1x0arVrVYYC1gXngQ>