igb and ALTQ in 9.1-rc3
Nick Rogers
ncrogers at gmail.com
Tue Apr 2 16:17:23 UTC 2013
On Tue, Apr 2, 2013 at 7:47 AM, Karim Fodil-Lemelin
<fodillemlinkarim at 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.
>
> 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 at 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 at gmail.com> wrote:
>>>
>>> On Tue, Dec 11, 2012 at 1:09 AM, Jack Vogel <jfvogel at gmail.com> wrote:
>>>
>>> On Mon, Dec 10, 2012 at 11:58 PM, Gleb Smirnoff <glebius at 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 at freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-net
>> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"
>
>
More information about the freebsd-net
mailing list