From owner-freebsd-net@FreeBSD.ORG Tue Apr 2 21:50:01 2013 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 82587D3B for ; Tue, 2 Apr 2013 21:50:01 +0000 (UTC) (envelope-from ncrogers@gmail.com) Received: from mail-ve0-f177.google.com (mail-ve0-f177.google.com [209.85.128.177]) by mx1.freebsd.org (Postfix) with ESMTP id 426E4D98 for ; Tue, 2 Apr 2013 21:50:00 +0000 (UTC) Received: by mail-ve0-f177.google.com with SMTP id jw11so1137214veb.8 for ; Tue, 02 Apr 2013 14:49:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=jzI6CX7liwtkwG3j8aQXAJg/HxAG3eZc0AesRZ8kaWU=; b=mlEvsJ3HpJ1ewVYprXx+KHHXJMRl5xPRiNE14eHBToBxdonisFBU63uoR6FBd3upuA atfgPK7FE5LRcXncfkakq2Bijtsh/2OK8eCl2SMIwC9E7K/iUESQBtnbdZxad4X5JLi/ 9kyR2vAyNpeRJk1PaUkSHsF1mSdDibyfMPTwMt5VPdgkykNw1sHJFUa55XmOTvVkSwNR yUAleSHlWPmajqYld2B0usgFhhA3L+gn5SaYGjxHvR8mgJxKs5I3LyYuoh6dHWfzCmo+ 2BTkm7AH8Hez5igEBzpW6xr/8PSmV6IpHALWg4OmqmH5KwCvS99DupoanViKlbP8koIr aB3w== MIME-Version: 1.0 X-Received: by 10.58.188.48 with SMTP id fx16mr14177466vec.22.1364939394699; Tue, 02 Apr 2013 14:49:54 -0700 (PDT) Received: by 10.52.176.131 with HTTP; Tue, 2 Apr 2013 14:49:54 -0700 (PDT) In-Reply-To: <515B46B8.5050806@gmail.com> References: <1355177348.71087.YahooMailClassic@web121601.mail.ne1.yahoo.com> <50C6656E.3020601@gmail.com> <20121211075853.GU48639@FreeBSD.org> <5159AB72.1050202@gmail.com> <515ADACD.8010108@gmail.com> <515AEF67.4070206@gmail.com> <515B46B8.5050806@gmail.com> Date: Tue, 2 Apr 2013 14:49:54 -0700 Message-ID: Subject: Re: igb and ALTQ in 9.1-rc3 From: Nick Rogers To: Karim Fodil-Lemelin Content-Type: text/plain; charset=ISO-8859-1 Cc: "freebsd-net@freebsd.org" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Apr 2013 21:50:01 -0000 On Tue, Apr 2, 2013 at 1:59 PM, Karim Fodil-Lemelin 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 wrote: >>> >>> On Tue, Apr 2, 2013 at 7:47 AM, Karim Fodil-Lemelin >>> 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 >>>>>> 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.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 >>>>>> wrote: >>>>>> >>>>>> On Tue, Dec 11, 2012 at 1:09 AM, Jack Vogel >>>>>> wrote: >>>>>> >>>>>> On Mon, Dec 10, 2012 at 11:58 PM, Gleb Smirnoff >>>>>> >>>>>> 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" > >