Re: add support for multiple transmit queues on interfaces
I did some tests.
The performance did not change.
I think this is the expected behaviour.
BR
Simon
2017-01-23 7:35 GMT+01:00, David Gwynne :
> hrvoje popovski hit a problem where the kernel would panic under load.
>
> i mistakenly called an interfaces qstart routine directly from
> if_enqueue rather than via the ifq serializer. this meant that txeof
> routines on network cards calling ifq_restart would cause the start
> routine to run concurrently, therefore causing corruption of the
> ring state.
>
> this diff fixes that.
>
> On Mon, Jan 23, 2017 at 01:09:57PM +1000, David Gwynne wrote:
>> the short explanation is that this lets interfaces allocate multiple
>> ifq structures that can be mapped to their transmit rings. the
>> mechanism for this is a driver calling if_attach_queues() after
>> theyve called if_attach().
>>
>> the long version is that this has if_enqueue access an array of
>> ifqueues on the interface instead of if_snd directly. the ifq is
>> picked by asking the queue discipline (priq or hfsc) to map an mbuf
>> to a slot in the if_ifqs array.
>>
>> to notify the driver that a particular queue needs to start ive
>> added a new function pointer to ifnet called if_qstart. if_qstart
>> takes an ifqueue * as an argument instead of an ifnet *, thereby
>> getting past the implicit behaviour that interfaces only have a
>> single ring.
>>
>> our drivers all have if_start routines that take ifnet pointers
>> though, so there's compatability for those where a default if_qstart
>> implementation calls if_start for those drivers. in the future
>> if_start will be replaced with if_qstart and we can rename it back
>> to if_start. until then, there's compat.
>>
>> drivers that provide their own if_qstart instead of an if_start
>> function notify the stack by setting IFXF_MPSAFE. a chunk of this
>> diff is changing the IFXF_MPSAFE drivers to set if_qstart instead
>> of if_start. note that this is a mechanical change, it does not add
>> multiple tx queues to these drivers.
>>
>> most of this is straightforward except for the hfsc handling. hfsc
>> needs to track all flows going over an interface, which means all
>> flows have to be serialised through hfsc. the mechanism in use
>> before this change was to swap the priq backend on if_snd with the
>> hfsc backend. the trick with this diff is that we still do that,
>> ie, we only change the first ifqueue on an interface over to hfsc.
>> this works because we use the ifqops on the first ifq to map packets
>> to any of them. because the hfsc map function unconditionally maps
>> packets to the first ifq, all packets end up going through the one
>> hfsc structure we set up. the rest of the ifqs remain set up as
>> priq, but dont get used for sending packets after hfsc has been
>> enabled. if we ever add another ifqops backend, this will have to
>> be rethought. until then this is an elegant hack.
>>
>> a consequence of this change is that we the ifnet if_start function
>> should not be called anymore. this isnt true at the moment because
>> of things like net80211 and ppp. they both queue management packets
>> onto a separate queue, but those separate queues are dequeued and
>> processed in the interfaces start routine. if we want to mark wifi
>> and ppp drivers as mpsafe (or get rid of separate if_start and
>> if_qstart routines) this will have to change.
>>
>> the guts of this change are in if_enqueue and if_attach_queues.
>>
>> ok?
>>
>
> Index: arch/octeon/dev/if_cnmac.c
> ===
> RCS file: /cvs/src/sys/arch/octeon/dev/if_cnmac.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 if_cnmac.c
> --- arch/octeon/dev/if_cnmac.c5 Nov 2016 05:14:18 - 1.61
> +++ arch/octeon/dev/if_cnmac.c23 Jan 2017 06:32:59 -
> @@ -138,7 +138,7 @@ int octeon_eth_ioctl(struct ifnet *, u_l
> void octeon_eth_watchdog(struct ifnet *);
> int octeon_eth_init(struct ifnet *);
> int octeon_eth_stop(struct ifnet *, int);
> -void octeon_eth_start(struct ifnet *);
> +void octeon_eth_start(struct ifqueue *);
>
> int octeon_eth_send_cmd(struct octeon_eth_softc *, uint64_t, uint64_t);
> uint64_t octeon_eth_send_makecmd_w1(int, paddr_t);
> @@ -303,7 +303,7 @@ octeon_eth_attach(struct device *parent,
> ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> ifp->if_xflags = IFXF_MPSAFE;
> ifp->if_ioctl = octeon_eth_ioctl;
> - ifp->if_start = octeon_eth_start;
> + ifp->if_qstart = octeon_eth_start;
> ifp->if_watchdog = octeon_eth_watchdog;
> ifp->if_hardmtu = OCTEON_ETH_MAX_MTU;
> IFQ_SET_MAXLEN(&ifp->if_snd, max(GATHER_QUEUE_SIZE, IFQ_MAXLEN));
> @@ -704,8 +704,6 @@ octeon_eth_ioctl(struct ifnet *ifp, u_lo
> error = 0;
> }
>
> - if_start(ifp);
> -
> splx(s);
> return (error);
> }
> @@ -923,13 +921,14 @@ done:
> }
>
> void
> -octeon_eth_start(struct ifnet *ifp)
> +octeon_eth_start(struct ifqueue *ifq)
> {
>
Re: add support for multiple transmit queues on interfaces
hrvoje popovski hit a problem where the kernel would panic under load.
i mistakenly called an interfaces qstart routine directly from
if_enqueue rather than via the ifq serializer. this meant that txeof
routines on network cards calling ifq_restart would cause the start
routine to run concurrently, therefore causing corruption of the
ring state.
this diff fixes that.
On Mon, Jan 23, 2017 at 01:09:57PM +1000, David Gwynne wrote:
> the short explanation is that this lets interfaces allocate multiple
> ifq structures that can be mapped to their transmit rings. the
> mechanism for this is a driver calling if_attach_queues() after
> theyve called if_attach().
>
> the long version is that this has if_enqueue access an array of
> ifqueues on the interface instead of if_snd directly. the ifq is
> picked by asking the queue discipline (priq or hfsc) to map an mbuf
> to a slot in the if_ifqs array.
>
> to notify the driver that a particular queue needs to start ive
> added a new function pointer to ifnet called if_qstart. if_qstart
> takes an ifqueue * as an argument instead of an ifnet *, thereby
> getting past the implicit behaviour that interfaces only have a
> single ring.
>
> our drivers all have if_start routines that take ifnet pointers
> though, so there's compatability for those where a default if_qstart
> implementation calls if_start for those drivers. in the future
> if_start will be replaced with if_qstart and we can rename it back
> to if_start. until then, there's compat.
>
> drivers that provide their own if_qstart instead of an if_start
> function notify the stack by setting IFXF_MPSAFE. a chunk of this
> diff is changing the IFXF_MPSAFE drivers to set if_qstart instead
> of if_start. note that this is a mechanical change, it does not add
> multiple tx queues to these drivers.
>
> most of this is straightforward except for the hfsc handling. hfsc
> needs to track all flows going over an interface, which means all
> flows have to be serialised through hfsc. the mechanism in use
> before this change was to swap the priq backend on if_snd with the
> hfsc backend. the trick with this diff is that we still do that,
> ie, we only change the first ifqueue on an interface over to hfsc.
> this works because we use the ifqops on the first ifq to map packets
> to any of them. because the hfsc map function unconditionally maps
> packets to the first ifq, all packets end up going through the one
> hfsc structure we set up. the rest of the ifqs remain set up as
> priq, but dont get used for sending packets after hfsc has been
> enabled. if we ever add another ifqops backend, this will have to
> be rethought. until then this is an elegant hack.
>
> a consequence of this change is that we the ifnet if_start function
> should not be called anymore. this isnt true at the moment because
> of things like net80211 and ppp. they both queue management packets
> onto a separate queue, but those separate queues are dequeued and
> processed in the interfaces start routine. if we want to mark wifi
> and ppp drivers as mpsafe (or get rid of separate if_start and
> if_qstart routines) this will have to change.
>
> the guts of this change are in if_enqueue and if_attach_queues.
>
> ok?
>
Index: arch/octeon/dev/if_cnmac.c
===
RCS file: /cvs/src/sys/arch/octeon/dev/if_cnmac.c,v
retrieving revision 1.61
diff -u -p -r1.61 if_cnmac.c
--- arch/octeon/dev/if_cnmac.c 5 Nov 2016 05:14:18 - 1.61
+++ arch/octeon/dev/if_cnmac.c 23 Jan 2017 06:32:59 -
@@ -138,7 +138,7 @@ int octeon_eth_ioctl(struct ifnet *, u_l
void octeon_eth_watchdog(struct ifnet *);
intocteon_eth_init(struct ifnet *);
intocteon_eth_stop(struct ifnet *, int);
-void octeon_eth_start(struct ifnet *);
+void octeon_eth_start(struct ifqueue *);
intocteon_eth_send_cmd(struct octeon_eth_softc *, uint64_t, uint64_t);
uint64_t octeon_eth_send_makecmd_w1(int, paddr_t);
@@ -303,7 +303,7 @@ octeon_eth_attach(struct device *parent,
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
ifp->if_xflags = IFXF_MPSAFE;
ifp->if_ioctl = octeon_eth_ioctl;
- ifp->if_start = octeon_eth_start;
+ ifp->if_qstart = octeon_eth_start;
ifp->if_watchdog = octeon_eth_watchdog;
ifp->if_hardmtu = OCTEON_ETH_MAX_MTU;
IFQ_SET_MAXLEN(&ifp->if_snd, max(GATHER_QUEUE_SIZE, IFQ_MAXLEN));
@@ -704,8 +704,6 @@ octeon_eth_ioctl(struct ifnet *ifp, u_lo
error = 0;
}
- if_start(ifp);
-
splx(s);
return (error);
}
@@ -923,13 +921,14 @@ done:
}
void
-octeon_eth_start(struct ifnet *ifp)
+octeon_eth_start(struct ifqueue *ifq)
{
+ struct ifnet *ifp = ifq->ifq_if;
struct octeon_eth_softc *sc = ifp->if_softc;
struct mbuf *m;
if (__predict_false(!cn30xxgmx_link_status(sc->sc_gmx_port))) {
- ifq_purge(&ifp->if_snd);
+ ifq_purge(ifq);
add support for multiple transmit queues on interfaces
the short explanation is that this lets interfaces allocate multiple
ifq structures that can be mapped to their transmit rings. the
mechanism for this is a driver calling if_attach_queues() after
theyve called if_attach().
the long version is that this has if_enqueue access an array of
ifqueues on the interface instead of if_snd directly. the ifq is
picked by asking the queue discipline (priq or hfsc) to map an mbuf
to a slot in the if_ifqs array.
to notify the driver that a particular queue needs to start ive
added a new function pointer to ifnet called if_qstart. if_qstart
takes an ifqueue * as an argument instead of an ifnet *, thereby
getting past the implicit behaviour that interfaces only have a
single ring.
our drivers all have if_start routines that take ifnet pointers
though, so there's compatability for those where a default if_qstart
implementation calls if_start for those drivers. in the future
if_start will be replaced with if_qstart and we can rename it back
to if_start. until then, there's compat.
drivers that provide their own if_qstart instead of an if_start
function notify the stack by setting IFXF_MPSAFE. a chunk of this
diff is changing the IFXF_MPSAFE drivers to set if_qstart instead
of if_start. note that this is a mechanical change, it does not add
multiple tx queues to these drivers.
most of this is straightforward except for the hfsc handling. hfsc
needs to track all flows going over an interface, which means all
flows have to be serialised through hfsc. the mechanism in use
before this change was to swap the priq backend on if_snd with the
hfsc backend. the trick with this diff is that we still do that,
ie, we only change the first ifqueue on an interface over to hfsc.
this works because we use the ifqops on the first ifq to map packets
to any of them. because the hfsc map function unconditionally maps
packets to the first ifq, all packets end up going through the one
hfsc structure we set up. the rest of the ifqs remain set up as
priq, but dont get used for sending packets after hfsc has been
enabled. if we ever add another ifqops backend, this will have to
be rethought. until then this is an elegant hack.
a consequence of this change is that we the ifnet if_start function
should not be called anymore. this isnt true at the moment because
of things like net80211 and ppp. they both queue management packets
onto a separate queue, but those separate queues are dequeued and
processed in the interfaces start routine. if we want to mark wifi
and ppp drivers as mpsafe (or get rid of separate if_start and
if_qstart routines) this will have to change.
the guts of this change are in if_enqueue and if_attach_queues.
ok?
Index: arch/octeon/dev/if_cnmac.c
===
RCS file: /cvs/src/sys/arch/octeon/dev/if_cnmac.c,v
retrieving revision 1.61
diff -u -p -r1.61 if_cnmac.c
--- arch/octeon/dev/if_cnmac.c 5 Nov 2016 05:14:18 - 1.61
+++ arch/octeon/dev/if_cnmac.c 23 Jan 2017 02:00:53 -
@@ -138,7 +138,7 @@ int octeon_eth_ioctl(struct ifnet *, u_l
void octeon_eth_watchdog(struct ifnet *);
intocteon_eth_init(struct ifnet *);
intocteon_eth_stop(struct ifnet *, int);
-void octeon_eth_start(struct ifnet *);
+void octeon_eth_start(struct ifqueue *);
intocteon_eth_send_cmd(struct octeon_eth_softc *, uint64_t, uint64_t);
uint64_t octeon_eth_send_makecmd_w1(int, paddr_t);
@@ -303,7 +303,7 @@ octeon_eth_attach(struct device *parent,
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
ifp->if_xflags = IFXF_MPSAFE;
ifp->if_ioctl = octeon_eth_ioctl;
- ifp->if_start = octeon_eth_start;
+ ifp->if_qstart = octeon_eth_start;
ifp->if_watchdog = octeon_eth_watchdog;
ifp->if_hardmtu = OCTEON_ETH_MAX_MTU;
IFQ_SET_MAXLEN(&ifp->if_snd, max(GATHER_QUEUE_SIZE, IFQ_MAXLEN));
@@ -704,8 +704,6 @@ octeon_eth_ioctl(struct ifnet *ifp, u_lo
error = 0;
}
- if_start(ifp);
-
splx(s);
return (error);
}
@@ -923,13 +921,14 @@ done:
}
void
-octeon_eth_start(struct ifnet *ifp)
+octeon_eth_start(struct ifqueue *ifq)
{
+ struct ifnet *ifp = ifq->ifq_if;
struct octeon_eth_softc *sc = ifp->if_softc;
struct mbuf *m;
if (__predict_false(!cn30xxgmx_link_status(sc->sc_gmx_port))) {
- ifq_purge(&ifp->if_snd);
+ ifq_purge(ifq);
return;
}
@@ -948,12 +947,12 @@ octeon_eth_start(struct ifnet *ifp)
* and bail out.
*/
if (octeon_eth_send_queue_is_full(sc)) {
- ifq_set_oactive(&ifp->if_snd);
+ ifq_set_oactive(ifq);
timeout_add(&sc->sc_tick_free_ch, 1);
return;
}
- m = ifq_dequeue(&ifp->if_snd);
+ m = ifq_dequeue(ifq);
if (m == NULL)
