Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 02 Apr 2013 16:59:36 -0400
From:      Karim Fodil-Lemelin <fodillemlinkarim@gmail.com>
To:        Nick Rogers <ncrogers@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:  <515B46B8.5050806@gmail.com>
In-Reply-To: <CAKOb=Yan9b2LS_WbsMwQEkkN0%2BWRpx8eC1x0arVrVYYC1gXngQ@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> <CAKOb=Yan9b2LS_WbsMwQEkkN0%2BWRpx8eC1x0arVrVYYC1gXngQ@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <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?515B46B8.5050806>