Date: Tue, 2 Apr 2013 14:49:54 -0700 From: Nick Rogers <ncrogers@gmail.com> To: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org> Subject: Re: igb and ALTQ in 9.1-rc3 Message-ID: <CAKOb=YZA6m5UrBiTmMcx6dTYzK4f67%2BUZYOEN_Lj%2Bi9_pMcCRQ@mail.gmail.com> In-Reply-To: <515B46B8.5050806@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> <CAKOb=Yan9b2LS_WbsMwQEkkN0%2BWRpx8eC1x0arVrVYYC1gXngQ@mail.gmail.com> <515B46B8.5050806@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Apr 2, 2013 at 1:59 PM, Karim Fodil-Lemelin
<fodillemlinkarim@gmail.com> wrote:
> 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.
Yes I am aware of this, thanks.
Also, FWIW, the changes Jack made to e1000 in HEAD are working for me
now and they should get MFCd eventually. If you set IGB_LEGACY_TX, the
multiqueue stack is skipped in favor of the old stuff which enables
ALTQ to work correctly again. This seems like it may be a cleaner
approach than the patch you suggested.
I am unsure the best way to set IGB_LEGACY_TX without adding a #define
IGB_LEGACY_TX to if_igb.c. Adding this to CFLAGS in make.conf and/or
the kernel conf does not seem to take effect when building the driver
into the kernel (i.e., not as a module). Can anyone comment on a
correct way to set this macro without modifying the driver code?
>
> Best regards,
>
> Karim.
>
>
> On 02/04/2013 1:17 PM, Nick Rogers wrote:
>>
>> 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"
>>>>
>>>>
>> _______________________________________________
>> 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=YZA6m5UrBiTmMcx6dTYzK4f67%2BUZYOEN_Lj%2Bi9_pMcCRQ>