interface queue transmit mitigation (again)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

interface queue transmit mitigation (again)

David Gwynne-5
this adds transmit mitigation back to the tree.

it is basically the same diff as last time. the big difference this
time is that all the tunnel drivers all defer ip_output calls, which
avoids having to play games with NET_LOCK in the ifq transmit paths.

tests? ok?

Index: ifq.c
===================================================================
RCS file: /cvs/src/sys/net/ifq.c,v
retrieving revision 1.22
diff -u -p -r1.22 ifq.c
--- ifq.c 25 Jan 2018 14:04:36 -0000 1.22
+++ ifq.c 14 Mar 2018 02:58:13 -0000
@@ -70,9 +70,16 @@ struct priq {
 void ifq_start_task(void *);
 void ifq_restart_task(void *);
 void ifq_barrier_task(void *);
+void ifq_bundle_task(void *);
 
 #define TASK_ONQUEUE 0x1
 
+static inline void
+ifq_run_start(struct ifqueue *ifq)
+{
+ ifq_serialize(ifq, &ifq->ifq_start);
+}
+
 void
 ifq_serialize(struct ifqueue *ifq, struct task *t)
 {
@@ -114,6 +121,16 @@ ifq_is_serialized(struct ifqueue *ifq)
 }
 
 void
+ifq_start(struct ifqueue *ifq)
+{
+ if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {
+ task_del(ifq->ifq_softnet, &ifq->ifq_bundle);
+ ifq_run_start(ifq);
+ } else
+ task_add(ifq->ifq_softnet, &ifq->ifq_bundle);
+}
+
+void
 ifq_start_task(void *p)
 {
  struct ifqueue *ifq = p;
@@ -137,11 +154,33 @@ ifq_restart_task(void *p)
 }
 
 void
+ifq_bundle_task(void *p)
+{
+ struct ifqueue *ifq = p;
+
+ ifq_run_start(ifq);
+}
+
+void
 ifq_barrier(struct ifqueue *ifq)
 {
  struct cond c = COND_INITIALIZER();
  struct task t = TASK_INITIALIZER(ifq_barrier_task, &c);
 
+ NET_ASSERT_UNLOCKED();
+
+ if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) {
+ int netlocked = (rw_status(&netlock) == RW_WRITE);
+
+ if (netlocked) /* XXXSMP breaks atomicity */
+ NET_UNLOCK();
+
+ taskq_barrier(ifq->ifq_softnet);
+
+ if (netlocked)
+ NET_LOCK();
+ }
+
  if (ifq->ifq_serializer == NULL)
  return;
 
@@ -166,6 +205,7 @@ void
 ifq_init(struct ifqueue *ifq, struct ifnet *ifp, unsigned int idx)
 {
  ifq->ifq_if = ifp;
+ ifq->ifq_softnet = net_tq(ifp->if_index);
  ifq->ifq_softc = NULL;
 
  mtx_init(&ifq->ifq_mtx, IPL_NET);
@@ -187,6 +227,7 @@ ifq_init(struct ifqueue *ifq, struct ifn
  mtx_init(&ifq->ifq_task_mtx, IPL_NET);
  TAILQ_INIT(&ifq->ifq_task_list);
  ifq->ifq_serializer = NULL;
+ task_set(&ifq->ifq_bundle, ifq_bundle_task, ifq);
 
  task_set(&ifq->ifq_start, ifq_start_task, ifq);
  task_set(&ifq->ifq_restart, ifq_restart_task, ifq);
@@ -237,6 +278,8 @@ void
 ifq_destroy(struct ifqueue *ifq)
 {
  struct mbuf_list ml = MBUF_LIST_INITIALIZER();
+
+ ifq_barrier(ifq); /* ensure nothing is running with the ifq */
 
  /* don't need to lock because this is the last use of the ifq */
 
Index: ifq.h
===================================================================
RCS file: /cvs/src/sys/net/ifq.h,v
retrieving revision 1.20
diff -u -p -r1.20 ifq.h
--- ifq.h 4 Jan 2018 11:02:57 -0000 1.20
+++ ifq.h 14 Mar 2018 02:58:13 -0000
@@ -25,6 +25,7 @@ struct ifq_ops;
 
 struct ifqueue {
  struct ifnet *ifq_if;
+ struct taskq *ifq_softnet;
  union {
  void *_ifq_softc;
  /*
@@ -57,6 +58,7 @@ struct ifqueue {
  struct mutex ifq_task_mtx;
  struct task_list ifq_task_list;
  void *ifq_serializer;
+ struct task ifq_bundle;
 
  /* work to be serialised */
  struct task ifq_start;
@@ -405,6 +407,7 @@ void ifq_attach(struct ifqueue *, cons
 void ifq_destroy(struct ifqueue *);
 void ifq_add_data(struct ifqueue *, struct if_data *);
 int ifq_enqueue(struct ifqueue *, struct mbuf *);
+void ifq_start(struct ifqueue *);
 struct mbuf *ifq_deq_begin(struct ifqueue *);
 void ifq_deq_commit(struct ifqueue *, struct mbuf *);
 void ifq_deq_rollback(struct ifqueue *, struct mbuf *);
@@ -438,12 +441,6 @@ static inline unsigned int
 ifq_is_oactive(struct ifqueue *ifq)
 {
  return (ifq->ifq_oactive);
-}
-
-static inline void
-ifq_start(struct ifqueue *ifq)
-{
- ifq_serialize(ifq, &ifq->ifq_start);
 }
 
 static inline void

Reply | Threaded
Open this post in threaded view
|

Re: interface queue transmit mitigation (again)

Martin Pieuchot
On 14/03/18(Wed) 13:00, David Gwynne wrote:
> this adds transmit mitigation back to the tree.
>
> it is basically the same diff as last time. the big difference this
> time is that all the tunnel drivers all defer ip_output calls, which
> avoids having to play games with NET_LOCK in the ifq transmit paths.

Comments inline.

> Index: ifq.c
> ===================================================================
> RCS file: /cvs/src/sys/net/ifq.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 ifq.c
> --- ifq.c 25 Jan 2018 14:04:36 -0000 1.22
> +++ ifq.c 14 Mar 2018 02:58:13 -0000
> @@ -70,9 +70,16 @@ struct priq {
>  void ifq_start_task(void *);
>  void ifq_restart_task(void *);
>  void ifq_barrier_task(void *);
> +void ifq_bundle_task(void *);
>  
>  #define TASK_ONQUEUE 0x1
>  
> +static inline void
> +ifq_run_start(struct ifqueue *ifq)
> +{
> + ifq_serialize(ifq, &ifq->ifq_start);
> +}
> +
>  void
>  ifq_serialize(struct ifqueue *ifq, struct task *t)
>  {
> @@ -114,6 +121,16 @@ ifq_is_serialized(struct ifqueue *ifq)
>  }
>  
>  void
> +ifq_start(struct ifqueue *ifq)
> +{
> + if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {

Why 4?  DragonFly recently bumped `ifsq_stage_cntmax' to 16.  Did you
try other values?  They also have an XXX comment that this value should
be per-interface.  Why?

In any case I'd be happier with a define.

> + task_del(ifq->ifq_softnet, &ifq->ifq_bundle);
> + ifq_run_start(ifq);
> + } else
> + task_add(ifq->ifq_softnet, &ifq->ifq_bundle);
> +}
> +
> +void
>  ifq_start_task(void *p)
>  {
>   struct ifqueue *ifq = p;
> @@ -137,11 +154,33 @@ ifq_restart_task(void *p)
>  }
>  
>  void
> +ifq_bundle_task(void *p)
> +{
> + struct ifqueue *ifq = p;
> +
> + ifq_run_start(ifq);
> +}
> +
> +void
>  ifq_barrier(struct ifqueue *ifq)
>  {
>   struct cond c = COND_INITIALIZER();
>   struct task t = TASK_INITIALIZER(ifq_barrier_task, &c);
>  
> + NET_ASSERT_UNLOCKED();

Since you assert here...

> +
> + if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) {
> + int netlocked = (rw_status(&netlock) == RW_WRITE);
                    ^^^^^^^^^
You can remove this code.  

> +
> + if (netlocked) /* XXXSMP breaks atomicity */
> + NET_UNLOCK();
> +
> + taskq_barrier(ifq->ifq_softnet);
> +
> + if (netlocked)
> + NET_LOCK();
> + }
> +
>   if (ifq->ifq_serializer == NULL)
>   return;
>  
> @@ -166,6 +205,7 @@ void
>  ifq_init(struct ifqueue *ifq, struct ifnet *ifp, unsigned int idx)
>  {
>   ifq->ifq_if = ifp;
> + ifq->ifq_softnet = net_tq(ifp->if_index);
>   ifq->ifq_softc = NULL;
>  
>   mtx_init(&ifq->ifq_mtx, IPL_NET);
> @@ -187,6 +227,7 @@ ifq_init(struct ifqueue *ifq, struct ifn
>   mtx_init(&ifq->ifq_task_mtx, IPL_NET);
>   TAILQ_INIT(&ifq->ifq_task_list);
>   ifq->ifq_serializer = NULL;
> + task_set(&ifq->ifq_bundle, ifq_bundle_task, ifq);
>  
>   task_set(&ifq->ifq_start, ifq_start_task, ifq);
>   task_set(&ifq->ifq_restart, ifq_restart_task, ifq);
> @@ -237,6 +278,8 @@ void
>  ifq_destroy(struct ifqueue *ifq)
>  {
>   struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> +
> + ifq_barrier(ifq); /* ensure nothing is running with the ifq */
>  
>   /* don't need to lock because this is the last use of the ifq */
>  
> Index: ifq.h
> ===================================================================
> RCS file: /cvs/src/sys/net/ifq.h,v
> retrieving revision 1.20
> diff -u -p -r1.20 ifq.h
> --- ifq.h 4 Jan 2018 11:02:57 -0000 1.20
> +++ ifq.h 14 Mar 2018 02:58:13 -0000
> @@ -25,6 +25,7 @@ struct ifq_ops;
>  
>  struct ifqueue {
>   struct ifnet *ifq_if;
> + struct taskq *ifq_softnet;
>   union {
>   void *_ifq_softc;
>   /*
> @@ -57,6 +58,7 @@ struct ifqueue {
>   struct mutex ifq_task_mtx;
>   struct task_list ifq_task_list;
>   void *ifq_serializer;
> + struct task ifq_bundle;
>  
>   /* work to be serialised */
>   struct task ifq_start;
> @@ -405,6 +407,7 @@ void ifq_attach(struct ifqueue *, cons
>  void ifq_destroy(struct ifqueue *);
>  void ifq_add_data(struct ifqueue *, struct if_data *);
>  int ifq_enqueue(struct ifqueue *, struct mbuf *);
> +void ifq_start(struct ifqueue *);
>  struct mbuf *ifq_deq_begin(struct ifqueue *);
>  void ifq_deq_commit(struct ifqueue *, struct mbuf *);
>  void ifq_deq_rollback(struct ifqueue *, struct mbuf *);
> @@ -438,12 +441,6 @@ static inline unsigned int
>  ifq_is_oactive(struct ifqueue *ifq)
>  {
>   return (ifq->ifq_oactive);
> -}
> -
> -static inline void
> -ifq_start(struct ifqueue *ifq)
> -{
> - ifq_serialize(ifq, &ifq->ifq_start);
>  }
>  
>  static inline void
>

Reply | Threaded
Open this post in threaded view
|

Re: interface queue transmit mitigation (again)

David Gwynne-5
On Thu, Mar 15, 2018 at 03:25:46PM +0100, Martin Pieuchot wrote:

> On 14/03/18(Wed) 13:00, David Gwynne wrote:
> > this adds transmit mitigation back to the tree.
> >
> > it is basically the same diff as last time. the big difference this
> > time is that all the tunnel drivers all defer ip_output calls, which
> > avoids having to play games with NET_LOCK in the ifq transmit paths.
>
> Comments inline.
>
> > + if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {
>
> Why 4?  DragonFly recently bumped `ifsq_stage_cntmax' to 16.  Did you
> try other values?  They also have an XXX comment that this value should
> be per-interface.  Why?

their default was 4, and they'd done some research on it. if they
moved to 16 there would be a reason for it.

> In any case I'd be happier with a define.

i've taken your advice on board and made it per interface, defaulted
to 16 with a macro. after this i can add ioctls to report it (under
hwfeatures maybe) and change it with ifconfig.

> >  ifq_barrier(struct ifqueue *ifq)
> >  {
> >   struct cond c = COND_INITIALIZER();
> >   struct task t = TASK_INITIALIZER(ifq_barrier_task, &c);
> >  
> > + NET_ASSERT_UNLOCKED();
>
> Since you assert here...
>
> > +
> > + if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) {
> > + int netlocked = (rw_status(&netlock) == RW_WRITE);
>    ^^^^^^^^^
> You can remove this code.

i can't get rid of the assert :(

ifq_barrier is called from lots of places, eg, ifq_destroy and
ix_down. the former does not hold the net lock, but the latter does.
putting manual NET_UNLOCK calls around places like the latter is
too much work imo.

Index: if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.548
diff -u -p -r1.548 if.c
--- if.c 2 Mar 2018 15:52:11 -0000 1.548
+++ if.c 28 Mar 2018 01:28:03 -0000
@@ -607,6 +607,7 @@ if_attach_common(struct ifnet *ifp)
  ifp->if_snd.ifq_ifqs[0] = &ifp->if_snd;
  ifp->if_ifqs = ifp->if_snd.ifq_ifqs;
  ifp->if_nifqs = 1;
+ ifp->if_sndmit = IF_SNDMIT_DEFAULT;
 
  ifiq_init(&ifp->if_rcv, ifp, 0);
 
Index: if_var.h
===================================================================
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.89
diff -u -p -r1.89 if_var.h
--- if_var.h 10 Jan 2018 23:50:39 -0000 1.89
+++ if_var.h 28 Mar 2018 01:28:03 -0000
@@ -170,6 +170,7 @@ struct ifnet { /* and the entries */
  struct ifqueue **if_ifqs; /* [I] pointer to an array of sndqs */
  void (*if_qstart)(struct ifqueue *);
  unsigned int if_nifqs; /* [I] number of output queues */
+ unsigned int if_sndmit; /* [c] tx mitigation amount */
 
  struct ifiqueue if_rcv; /* rx/input queue */
  struct ifiqueue **if_iqs; /* [I] pointer to the array of iqs */
@@ -279,6 +280,9 @@ do { \
 #define IFQ_LEN(ifq) ifq_len(ifq)
 #define IFQ_IS_EMPTY(ifq) ifq_empty(ifq)
 #define IFQ_SET_MAXLEN(ifq, len) ifq_set_maxlen(ifq, len)
+
+#define IF_SNDMIT_MIN 1
+#define IF_SNDMIT_DEFAULT 16
 
 /* default interface priorities */
 #define IF_WIRED_DEFAULT_PRIORITY 0
Index: ifq.c
===================================================================
RCS file: /cvs/src/sys/net/ifq.c,v
retrieving revision 1.22
diff -u -p -r1.22 ifq.c
--- ifq.c 25 Jan 2018 14:04:36 -0000 1.22
+++ ifq.c 28 Mar 2018 01:28:03 -0000
@@ -70,9 +70,16 @@ struct priq {
 void ifq_start_task(void *);
 void ifq_restart_task(void *);
 void ifq_barrier_task(void *);
+void ifq_bundle_task(void *);
 
 #define TASK_ONQUEUE 0x1
 
+static inline void
+ifq_run_start(struct ifqueue *ifq)
+{
+ ifq_serialize(ifq, &ifq->ifq_start);
+}
+
 void
 ifq_serialize(struct ifqueue *ifq, struct task *t)
 {
@@ -114,6 +121,16 @@ ifq_is_serialized(struct ifqueue *ifq)
 }
 
 void
+ifq_start(struct ifqueue *ifq)
+{
+ if (ifq_len(ifq) >= min(ifq->ifq_if->if_sndmit, ifq->ifq_maxlen)) {
+ task_del(ifq->ifq_softnet, &ifq->ifq_bundle);
+ ifq_run_start(ifq);
+ } else
+ task_add(ifq->ifq_softnet, &ifq->ifq_bundle);
+}
+
+void
 ifq_start_task(void *p)
 {
  struct ifqueue *ifq = p;
@@ -137,11 +154,31 @@ ifq_restart_task(void *p)
 }
 
 void
+ifq_bundle_task(void *p)
+{
+ struct ifqueue *ifq = p;
+
+ ifq_run_start(ifq);
+}
+
+void
 ifq_barrier(struct ifqueue *ifq)
 {
  struct cond c = COND_INITIALIZER();
  struct task t = TASK_INITIALIZER(ifq_barrier_task, &c);
 
+ if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) {
+ int netlocked = (rw_status(&netlock) == RW_WRITE);
+
+ if (netlocked) /* XXXSMP breaks atomicity */
+ NET_UNLOCK();
+
+ taskq_barrier(ifq->ifq_softnet);
+
+ if (netlocked)
+ NET_LOCK();
+ }
+
  if (ifq->ifq_serializer == NULL)
  return;
 
@@ -166,6 +203,7 @@ void
 ifq_init(struct ifqueue *ifq, struct ifnet *ifp, unsigned int idx)
 {
  ifq->ifq_if = ifp;
+ ifq->ifq_softnet = net_tq(ifp->if_index);
  ifq->ifq_softc = NULL;
 
  mtx_init(&ifq->ifq_mtx, IPL_NET);
@@ -187,6 +225,7 @@ ifq_init(struct ifqueue *ifq, struct ifn
  mtx_init(&ifq->ifq_task_mtx, IPL_NET);
  TAILQ_INIT(&ifq->ifq_task_list);
  ifq->ifq_serializer = NULL;
+ task_set(&ifq->ifq_bundle, ifq_bundle_task, ifq);
 
  task_set(&ifq->ifq_start, ifq_start_task, ifq);
  task_set(&ifq->ifq_restart, ifq_restart_task, ifq);
@@ -237,6 +276,8 @@ void
 ifq_destroy(struct ifqueue *ifq)
 {
  struct mbuf_list ml = MBUF_LIST_INITIALIZER();
+
+ ifq_barrier(ifq); /* ensure nothing is running with the ifq */
 
  /* don't need to lock because this is the last use of the ifq */
 
Index: ifq.h
===================================================================
RCS file: /cvs/src/sys/net/ifq.h,v
retrieving revision 1.20
diff -u -p -r1.20 ifq.h
--- ifq.h 4 Jan 2018 11:02:57 -0000 1.20
+++ ifq.h 28 Mar 2018 01:28:03 -0000
@@ -25,6 +25,7 @@ struct ifq_ops;
 
 struct ifqueue {
  struct ifnet *ifq_if;
+ struct taskq *ifq_softnet;
  union {
  void *_ifq_softc;
  /*
@@ -57,6 +58,7 @@ struct ifqueue {
  struct mutex ifq_task_mtx;
  struct task_list ifq_task_list;
  void *ifq_serializer;
+ struct task ifq_bundle;
 
  /* work to be serialised */
  struct task ifq_start;
@@ -405,6 +407,7 @@ void ifq_attach(struct ifqueue *, cons
 void ifq_destroy(struct ifqueue *);
 void ifq_add_data(struct ifqueue *, struct if_data *);
 int ifq_enqueue(struct ifqueue *, struct mbuf *);
+void ifq_start(struct ifqueue *);
 struct mbuf *ifq_deq_begin(struct ifqueue *);
 void ifq_deq_commit(struct ifqueue *, struct mbuf *);
 void ifq_deq_rollback(struct ifqueue *, struct mbuf *);
@@ -438,12 +441,6 @@ static inline unsigned int
 ifq_is_oactive(struct ifqueue *ifq)
 {
  return (ifq->ifq_oactive);
-}
-
-static inline void
-ifq_start(struct ifqueue *ifq)
-{
- ifq_serialize(ifq, &ifq->ifq_start);
 }
 
 static inline void

Reply | Threaded
Open this post in threaded view
|

Re: interface queue transmit mitigation (again)

Michael Price
On Tue, Mar 27, 2018 at 9:30 PM David Gwynne <[hidden email]> wrote:

> On Thu, Mar 15, 2018 at 03:25:46PM +0100, Martin Pieuchot wrote:
> > On 14/03/18(Wed) 13:00, David Gwynne wrote:
> > > this adds transmit mitigation back to the tree.
> > >
> > > it is basically the same diff as last time. the big difference this
> > > time is that all the tunnel drivers all defer ip_output calls, which
> > > avoids having to play games with NET_LOCK in the ifq transmit paths.
> >
> > Comments inline.
> >
> > > +   if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {
> >
> > Why 4?  DragonFly recently bumped `ifsq_stage_cntmax' to 16.  Did you
> > try other values?  They also have an XXX comment that this value should
> > be per-interface.  Why?
>
> their default was 4, and they'd done some research on it. if they
> moved to 16 there would be a reason for it.


Would it be this commit?

https://marc.info/?l=dragonfly-commits&m=151401707632544&w=2

Comments include test data.

Michael
Reply | Threaded
Open this post in threaded view
|

Re: interface queue transmit mitigation (again)

Martin Pieuchot
In reply to this post by David Gwynne-5
On 28/03/18(Wed) 11:28, David Gwynne wrote:

> On Thu, Mar 15, 2018 at 03:25:46PM +0100, Martin Pieuchot wrote:
> > On 14/03/18(Wed) 13:00, David Gwynne wrote:
> > > this adds transmit mitigation back to the tree.
> > >
> > > it is basically the same diff as last time. the big difference this
> > > time is that all the tunnel drivers all defer ip_output calls, which
> > > avoids having to play games with NET_LOCK in the ifq transmit paths.
> >
> > Comments inline.
> >
> > > + if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {
> >
> > Why 4?  DragonFly recently bumped `ifsq_stage_cntmax' to 16.  Did you
> > try other values?  They also have an XXX comment that this value should
> > be per-interface.  Why?
>
> their default was 4, and they'd done some research on it. if they
> moved to 16 there would be a reason for it.
>
> > In any case I'd be happier with a define.
>
> i've taken your advice on board and made it per interface, defaulted
> to 16 with a macro. after this i can add ioctls to report it (under
> hwfeatures maybe) and change it with ifconfig.
>
> > >  ifq_barrier(struct ifqueue *ifq)
> > >  {
> > >   struct cond c = COND_INITIALIZER();
> > >   struct task t = TASK_INITIALIZER(ifq_barrier_task, &c);
> > >  
> > > + NET_ASSERT_UNLOCKED();
> >
> > Since you assert here...
> >
> > > +
> > > + if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) {
> > > + int netlocked = (rw_status(&netlock) == RW_WRITE);
> >    ^^^^^^^^^
> > You can remove this code.
>
> i can't get rid of the assert :(
>
> ifq_barrier is called from lots of places, eg, ifq_destroy and
> ix_down. the former does not hold the net lock, but the latter does.
> putting manual NET_UNLOCK calls around places like the latter is
> too much work imo.

You're right, getting rid of the NET_LOCK() is too much work.  That's
why I'm being rude for such pattern not to spread.  I'd prefer an
assert and move the dance in the only place that needs it.

Anyway `if_sndmit' should be [d] since its a driver variable, not a
stack one.

Apart from that ok mpi@

> retrieving revision 1.548
> diff -u -p -r1.548 if.c
> --- if.c 2 Mar 2018 15:52:11 -0000 1.548
> +++ if.c 28 Mar 2018 01:28:03 -0000
> @@ -607,6 +607,7 @@ if_attach_common(struct ifnet *ifp)
>   ifp->if_snd.ifq_ifqs[0] = &ifp->if_snd;
>   ifp->if_ifqs = ifp->if_snd.ifq_ifqs;
>   ifp->if_nifqs = 1;
> + ifp->if_sndmit = IF_SNDMIT_DEFAULT;
>  
>   ifiq_init(&ifp->if_rcv, ifp, 0);
>  
> Index: if_var.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if_var.h,v
> retrieving revision 1.89
> diff -u -p -r1.89 if_var.h
> --- if_var.h 10 Jan 2018 23:50:39 -0000 1.89
> +++ if_var.h 28 Mar 2018 01:28:03 -0000
> @@ -170,6 +170,7 @@ struct ifnet { /* and the entries */
>   struct ifqueue **if_ifqs; /* [I] pointer to an array of sndqs */
>   void (*if_qstart)(struct ifqueue *);
>   unsigned int if_nifqs; /* [I] number of output queues */
> + unsigned int if_sndmit; /* [c] tx mitigation amount */
>  
>   struct ifiqueue if_rcv; /* rx/input queue */
>   struct ifiqueue **if_iqs; /* [I] pointer to the array of iqs */
> @@ -279,6 +280,9 @@ do { \
>  #define IFQ_LEN(ifq) ifq_len(ifq)
>  #define IFQ_IS_EMPTY(ifq) ifq_empty(ifq)
>  #define IFQ_SET_MAXLEN(ifq, len) ifq_set_maxlen(ifq, len)
> +
> +#define IF_SNDMIT_MIN 1
> +#define IF_SNDMIT_DEFAULT 16
>  
>  /* default interface priorities */
>  #define IF_WIRED_DEFAULT_PRIORITY 0
> Index: ifq.c
> ===================================================================
> RCS file: /cvs/src/sys/net/ifq.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 ifq.c
> --- ifq.c 25 Jan 2018 14:04:36 -0000 1.22
> +++ ifq.c 28 Mar 2018 01:28:03 -0000
> @@ -70,9 +70,16 @@ struct priq {
>  void ifq_start_task(void *);
>  void ifq_restart_task(void *);
>  void ifq_barrier_task(void *);
> +void ifq_bundle_task(void *);
>  
>  #define TASK_ONQUEUE 0x1
>  
> +static inline void
> +ifq_run_start(struct ifqueue *ifq)
> +{
> + ifq_serialize(ifq, &ifq->ifq_start);
> +}
> +
>  void
>  ifq_serialize(struct ifqueue *ifq, struct task *t)
>  {
> @@ -114,6 +121,16 @@ ifq_is_serialized(struct ifqueue *ifq)
>  }
>  
>  void
> +ifq_start(struct ifqueue *ifq)
> +{
> + if (ifq_len(ifq) >= min(ifq->ifq_if->if_sndmit, ifq->ifq_maxlen)) {
> + task_del(ifq->ifq_softnet, &ifq->ifq_bundle);
> + ifq_run_start(ifq);
> + } else
> + task_add(ifq->ifq_softnet, &ifq->ifq_bundle);
> +}
> +
> +void
>  ifq_start_task(void *p)
>  {
>   struct ifqueue *ifq = p;
> @@ -137,11 +154,31 @@ ifq_restart_task(void *p)
>  }
>  
>  void
> +ifq_bundle_task(void *p)
> +{
> + struct ifqueue *ifq = p;
> +
> + ifq_run_start(ifq);
> +}
> +
> +void
>  ifq_barrier(struct ifqueue *ifq)
>  {
>   struct cond c = COND_INITIALIZER();
>   struct task t = TASK_INITIALIZER(ifq_barrier_task, &c);
>  
> + if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) {
> + int netlocked = (rw_status(&netlock) == RW_WRITE);
> +
> + if (netlocked) /* XXXSMP breaks atomicity */
> + NET_UNLOCK();
> +
> + taskq_barrier(ifq->ifq_softnet);
> +
> + if (netlocked)
> + NET_LOCK();
> + }
> +
>   if (ifq->ifq_serializer == NULL)
>   return;
>  
> @@ -166,6 +203,7 @@ void
>  ifq_init(struct ifqueue *ifq, struct ifnet *ifp, unsigned int idx)
>  {
>   ifq->ifq_if = ifp;
> + ifq->ifq_softnet = net_tq(ifp->if_index);
>   ifq->ifq_softc = NULL;
>  
>   mtx_init(&ifq->ifq_mtx, IPL_NET);
> @@ -187,6 +225,7 @@ ifq_init(struct ifqueue *ifq, struct ifn
>   mtx_init(&ifq->ifq_task_mtx, IPL_NET);
>   TAILQ_INIT(&ifq->ifq_task_list);
>   ifq->ifq_serializer = NULL;
> + task_set(&ifq->ifq_bundle, ifq_bundle_task, ifq);
>  
>   task_set(&ifq->ifq_start, ifq_start_task, ifq);
>   task_set(&ifq->ifq_restart, ifq_restart_task, ifq);
> @@ -237,6 +276,8 @@ void
>  ifq_destroy(struct ifqueue *ifq)
>  {
>   struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> +
> + ifq_barrier(ifq); /* ensure nothing is running with the ifq */
>  
>   /* don't need to lock because this is the last use of the ifq */
>  
> Index: ifq.h
> ===================================================================
> RCS file: /cvs/src/sys/net/ifq.h,v
> retrieving revision 1.20
> diff -u -p -r1.20 ifq.h
> --- ifq.h 4 Jan 2018 11:02:57 -0000 1.20
> +++ ifq.h 28 Mar 2018 01:28:03 -0000
> @@ -25,6 +25,7 @@ struct ifq_ops;
>  
>  struct ifqueue {
>   struct ifnet *ifq_if;
> + struct taskq *ifq_softnet;
>   union {
>   void *_ifq_softc;
>   /*
> @@ -57,6 +58,7 @@ struct ifqueue {
>   struct mutex ifq_task_mtx;
>   struct task_list ifq_task_list;
>   void *ifq_serializer;
> + struct task ifq_bundle;
>  
>   /* work to be serialised */
>   struct task ifq_start;
> @@ -405,6 +407,7 @@ void ifq_attach(struct ifqueue *, cons
>  void ifq_destroy(struct ifqueue *);
>  void ifq_add_data(struct ifqueue *, struct if_data *);
>  int ifq_enqueue(struct ifqueue *, struct mbuf *);
> +void ifq_start(struct ifqueue *);
>  struct mbuf *ifq_deq_begin(struct ifqueue *);
>  void ifq_deq_commit(struct ifqueue *, struct mbuf *);
>  void ifq_deq_rollback(struct ifqueue *, struct mbuf *);
> @@ -438,12 +441,6 @@ static inline unsigned int
>  ifq_is_oactive(struct ifqueue *ifq)
>  {
>   return (ifq->ifq_oactive);
> -}
> -
> -static inline void
> -ifq_start(struct ifqueue *ifq)
> -{
> - ifq_serialize(ifq, &ifq->ifq_start);
>  }
>  
>  static inline void
>

Reply | Threaded
Open this post in threaded view
|

Re: interface queue transmit mitigation (again)

Hrvoje Popovski
In reply to this post by David Gwynne-5
On 28.3.2018. 3:28, David Gwynne wrote:

> On Thu, Mar 15, 2018 at 03:25:46PM +0100, Martin Pieuchot wrote:
>> On 14/03/18(Wed) 13:00, David Gwynne wrote:
>>> this adds transmit mitigation back to the tree.
>>>
>>> it is basically the same diff as last time. the big difference this
>>> time is that all the tunnel drivers all defer ip_output calls, which
>>> avoids having to play games with NET_LOCK in the ifq transmit paths.
>> Comments inline.
>>
>>> + if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {
>> Why 4?  DragonFly recently bumped `ifsq_stage_cntmax' to 16.  Did you
>> try other values?  They also have an XXX comment that this value should
>> be per-interface.  Why?
> their default was 4, and they'd done some research on it. if they
> moved to 16 there would be a reason for it.

Hi all,

with this diff i'm getting 1.43Mpps on
12 x Intel(R) Xeon(R) CPU E5-2620 v2 @ 2.10GHz, 2400.01 MHz

with plain kernel i'm getting 1.12Mpps and with older diff's i was
getting 1.31Mpps ...

if i execute ifconfig ix0 down while generating traffic over everything stop

x3550m4# ifconfig ix0 down
^C

from ddb:

ddb{0}> ps
   PID     TID   PPID    UID  S       FLAGS  WAIT          COMMAND
   287   54967   4723      0  3         0x3  tqbar         ifconfig
  4723  369240      1      0  3    0x10008b  pause         ksh
 78652   12963      1      0  3    0x100083  ttyin         getty
 77445  380566      1      0  3    0x100083  ttyin         getty
  7708   16964      1      0  3    0x100083  ttyin         getty
  6466  480278      1      0  3    0x100083  ttyin         getty
 10683  361200      1      0  3    0x100083  ttyin         ksh
  3322    2446      1      0  3    0x100098  poll          cron
  8187    8185      1     99  3    0x100090  poll          sndiod
 38828  292448      1    110  3    0x100090  poll          sndiod
 96602  349758  17921     95  3    0x100092  kqread        smtpd
 59159  244097  17921    103  3    0x100092  kqread        smtpd
 72520  357622  17921     95  3    0x100092  kqread        smtpd
 11196  442980  17921     95  3    0x100092  kqread        smtpd
 19374  184986  17921     95  3    0x100092  kqread        smtpd
 99758  239851  17921     95  3    0x100092  kqread        smtpd
 17921  468052      1      0  3    0x100080  kqread        smtpd
 96705  480305      1      0  3        0x80  select        sshd
 10784  513637  74642     83  3    0x100092  poll          ntpd
 74642  349129  19763     83  3    0x100092  poll          ntpd
 19763  118713      1      0  3    0x100080  poll          ntpd
 80820  281787  61744     73  3    0x100090  kqread        syslogd
 61744  358228      1      0  3    0x100082  netio         syslogd
 92438  103007  37416    115  3    0x100092  kqread        slaacd
 27600  284603  37416    115  3    0x100092  kqread        slaacd
 37416  302849      1      0  3        0x80  kqread        slaacd
 31025  461354      0      0  3     0x14200  pgzero        zerothread
 87259  136299      0      0  3     0x14200  aiodoned      aiodoned
 40842   63462      0      0  3     0x14200  syncer        update
 434254      0      0  3     0x14200  cleaner       cleaner
 20219  234861      0      0  3     0x14200  reaper        reaper
 49370  510675      0      0  3     0x14200  pgdaemon      pagedaemon
 34415  210813      0      0  3     0x14200  bored         crynlk
 98085  523911      0      0  3     0x14200  bored         crypto
 88352  390863      0      0  3     0x14200  usbtsk        usbtask
 36743   62252      0      0  3     0x14200  usbatsk       usbatsk
 38389  456819      0      0  3  0x40014200  acpi0         acpi0
 93162   81423      0      0  7  0x40014200                idle11
 58166   30480      0      0  7  0x40014200                idle10
 55398  115831      0      0  7  0x40014200                idle9
    96   42736      0      0  7  0x40014200                idle8
 63465  183206      0      0  7  0x40014200                idle7
 79804  340505      0      0  7  0x40014200                idle6
 42987  392463      0      0  7  0x40014200                idle5
 94478  284414      0      0  7  0x40014200                idle4
 45914   13592      0      0  7  0x40014200                idle3
 14508  513956      0      0  7  0x40014200                idle2
 16424  111283      0      0  7  0x40014200                idle1
 60252  298958      0      0  3     0x14200  bored         sensors
 80523  235128      0      0  3     0x14200  tqbar         softnet
 40231  252516      0      0  3     0x14200  bored         systqmp
 97996  128366      0      0  3     0x14200  bored         systq
 78639  112346      0      0  3  0x40014200  bored         softclock
*60124   95946      0      0  7  0x40014200                idle0
     1  232514      0      0  3        0x82  wait          init
     0       0     -1      0  3     0x10200  scheduler     swapper
ddb{0}>


ddb{0}> tr /p 0t54967
sleep_finish(ffff800023d3c528,ffffffff81a3863c) at sleep_finish+0x70
cond_wait(ffff800023d3c528,ffff800000025040) at cond_wait+0x51
taskq_barrier(ffff8000000df5d0) at taskq_barrier+0x86
ifq_barrier(ffff8000000df000) at ifq_barrier+0xbd
ixgbe_stop(ffff8000000df000) at ixgbe_stop+0x13f
ixgbe_ioctl(ffffff0784dec490,ffff8000000df048,80206910) at ixgbe_ioctl+0xaa
ifioctl(ffff800023d3c7e0,ffff8000ffff6b38,ffffff07857d1328,20) at
ifioctl+0x500

sys_ioctl(360,ffff8000ffff6b38,0) at sys_ioctl+0x346
syscall() at syscall+0x279
--- syscall (number 54) ---
end of kernel
end trace frame: 0x7f7ffffe18b0, count: -9
0xc3ba281f57a:


ddb{0}> tr /p 0t235128
sleep_finish(ffff800023c14558,ffffffff81a3863c) at sleep_finish+0x70
cond_wait(ffff800023c14558,ffff800000025040) at cond_wait+0x51
taskq_barrier(0) at taskq_barrier+0x86
ifq_barrier(ffff800000509000) at ifq_barrier+0xde
em_stop(ffff800000509000,7) at em_stop+0x62
em_init(2) at em_init+0x25
em_watchdog(0) at em_watchdog+0xc1
if_watchdog_task(ffffffff817118c0) at if_watchdog_task+0xa4
taskq_thread(0) at taskq_thread+0x67
end trace frame: 0x0, count: -9
ddb{0}>


Reply | Threaded
Open this post in threaded view
|

Re: interface queue transmit mitigation (again)

Hrvoje Popovski
On 28.3.2018. 11:42, Hrvoje Popovski wrote:
> Hi all,
>
> with this diff i'm getting 1.43Mpps on
> 12 x Intel(R) Xeon(R) CPU E5-2620 v2 @ 2.10GHz, 2400.01 MHz
>
> with plain kernel i'm getting 1.12Mpps and with older diff's i was
> getting 1.31Mpps ...

On box with 6 x Intel(R) Xeon(R) CPU E5-2643 v2 @ 3.50GHz, 3600.46 MHz
with or without this diff i'm getting 1.75 Mpps and i can't trigger
freeze although i left ifconfig down/up over weekend and it's still running.

Stability of forwarding performance seems more stable with this diff
than without it, meaning that there aren't oscillation in forwarding as
before.

Reply | Threaded
Open this post in threaded view
|

Re: interface queue transmit mitigation (again)

Hrvoje Popovski
In reply to this post by Hrvoje Popovski
On 28.3.2018. 11:42, Hrvoje Popovski wrote:

> On 28.3.2018. 3:28, David Gwynne wrote:
>> On Thu, Mar 15, 2018 at 03:25:46PM +0100, Martin Pieuchot wrote:
>>> On 14/03/18(Wed) 13:00, David Gwynne wrote:
>>>> this adds transmit mitigation back to the tree.
>>>>
>>>> it is basically the same diff as last time. the big difference this
>>>> time is that all the tunnel drivers all defer ip_output calls, which
>>>> avoids having to play games with NET_LOCK in the ifq transmit paths.
>>> Comments inline.
>>>
>>>> + if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {
>>> Why 4?  DragonFly recently bumped `ifsq_stage_cntmax' to 16.  Did you
>>> try other values?  They also have an XXX comment that this value should
>>> be per-interface.  Why?
>> their default was 4, and they'd done some research on it. if they
>> moved to 16 there would be a reason for it.
> Hi all,
>
> with this diff i'm getting 1.43Mpps on
> 12 x Intel(R) Xeon(R) CPU E5-2620 v2 @ 2.10GHz, 2400.01 MHz
>
> with plain kernel i'm getting 1.12Mpps and with older diff's i was
> getting 1.31Mpps ...
>
> if i execute ifconfig ix0 down while generating traffic over everything stop


I've tested this diff with today's tree and i can't repeat problem with
or without -fretpoline diff.