From owner-freebsd-net@FreeBSD.ORG Tue Apr 2 20:59:40 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 538BCB2F for ; Tue, 2 Apr 2013 20:59:40 +0000 (UTC) (envelope-from fodillemlinkarim@gmail.com) Received: from mail-ie0-x22b.google.com (mail-ie0-x22b.google.com [IPv6:2607:f8b0:4001:c03::22b]) by mx1.freebsd.org (Postfix) with ESMTP id 1F305A0D for ; Tue, 2 Apr 2013 20:59:40 +0000 (UTC) Received: by mail-ie0-f171.google.com with SMTP id e14so946306iej.2 for ; Tue, 02 Apr 2013 13:59:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=dqFSUqYH5Sf3g7bYDEC+f4Ol63O1stU68DNmE0wIoWE=; b=gjZuQSFT1wToHfGa6jZw9nRx5jAQkSOO182sYesw8Dpb2T/Qwwqp0kuZHZeXRtq+aA WRV3FZk/4YxYuGq/ghZ0X0sqLUbhwuY/2oJ0MVfMEzlB+8Me/ZQgogFeps8jdQ0tdU7u g3DD/Y7h9h/5fBSLOiP2SxDNkprasS5JwL8t075tpOM9zCx4F4iLOFNR6IRNKm1XIhJr ZY4547VHe/yDoQqjm9ARze2fVnUa1XpMWtQUfMCMwCYX+eEFxzpHuIaTFjSgzLwqBw+N TD8sCnqttgGKrRgr8oRdceYMyX2wlC9AUlp2kZcEemj2VVAkNXEsEhpMFiEdZcf0I7+o wjfQ== X-Received: by 10.50.49.66 with SMTP id s2mr751716ign.13.1364936379700; Tue, 02 Apr 2013 13:59:39 -0700 (PDT) Received: from [192.168.1.73] ([208.85.112.101]) by mx.google.com with ESMTPS id c14sm1694599ign.2.2013.04.02.13.59.37 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 02 Apr 2013 13:59:38 -0700 (PDT) Message-ID: <515B46B8.5050806@gmail.com> Date: Tue, 02 Apr 2013 16:59:36 -0400 From: Karim Fodil-Lemelin User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130307 Thunderbird/17.0.4 MIME-Version: 1.0 To: Nick Rogers Subject: Re: igb and ALTQ in 9.1-rc3 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> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "freebsd-net@freebsd.org" , "jfvogel@gmail.com" 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 20:59:40 -0000 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 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"