From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:22:50
Latest round of lockless qdisc patch set with performance metric
primarily using pktgen to inject pkts into the qdisc layer
This series introduces a flag to allow qdiscs to indicate they can run
without holding the qdisc lock. In order to set this bit most qdiscs
will need to be modified to use lockless data structures. This series
implements a lockless data structures for pfifo_fast by replacing the
skb list with an skb_array. This currently still uses spin locks to
protect the array which can be improved later.
Also its worth noting when the lockless bit is set we no longer use
the busy_lock in the tx qdisc path nor do we allow bypassing the
enqueue()/dequeue() operations. We can optimize this later as well
but I wanted to keep the initial series as straight forward as
possible. The benchmarks using pktgen do not indicate there is any
significant degradation from removing the bypass logic (see numbers
below).
Future work is the following,
- convert all qdiscs over to per cpu handling and cleanup the
rather ugly if/else statistics handling. Although a bit of
work its mechanical and should help some.
- I'm looking at fq_codel to see how to make it "lockless".
- It seems we can drop the TX_HARD_LOCK on cases where the
nic exposes a queue per core now that we have enqueue/dequeue
decoupled. The idea being a bunch of threads enqueue and per
core dequeue logic runs. Requires XPS to be setup.
- qlen improvements somehow
- look at improvements to the skb_array structure. We can look
at drop in replacements and/or improving it. For example the
dequeue spin locks are not needed in many cases.
Below is the data I took from pktgen,
./samples/pktgen/pktgen_bench_xmit_mode_queue_xmit.sh -t $NUM -i eth3
I did a run of 4 each time and took the total summation of each
thread. I did this for 1, 2, 4, 8, and 12 threads on both mqprio and
pfifo_fast. Overall pfifo_fast shows a performance improvement as the
number of threads increases which was causing contention in the
original locked version of the code. And on mq because I'm using an
Intel 10G hardware running the ixgbe driver creates a descriptor ring
per core resulting in pfifo_fast queue per core there is no
contention. As a result I do not see any performance improvement in
the benchmarks but it doesn't appear to hurt either so this is good.
nolock pfifo_fast
1: 1417597 1407479 1418913 1439601
2: 1882009 1867799 1864374 1855950
4: 1806736 1804261 1803697 1806994
8: 1354318 1358686 1353145 1356645
12: 1331928 1333079 1333476 1335544
locked pfifo_fast
1: 1471479 1469142 1458825 1456788
2: 1746231 1749490 1753176 1753780
4: 1119626 1120515 1121478 1119220
8: 1001471 999308 1000318 1000776
12: 989269 992122 991590 986581
nolock mq
1: 1417768 1438712 1449092 1426775
2: 2644099 2634961 2628939 2712867
4: 4866133 4862802 4863396 4867423
8: 9422061 9464986 9457825 9467619
12: 13854470 13213735 13664498 13213292
locked mq
1: 1448374 1444208 1437459 1437088
2: 2687963 2679221 2651059 2691630
4: 5153884 4684153 5091728 4635261
8: 9292395 9625869 9681835 9711651
12: 13553918 13682410 14084055 13946138
---
John Fastabend (15):
net: sched: cleanup qdisc_run and __qdisc_run semantics
net: sched: allow qdiscs to handle locking
net: sched: remove remaining uses for qdisc_qlen in xmit path
net: sched: provide per cpu qstat helpers
net: sched: a dflt qdisc may be used with per cpu stats
net: sched: per cpu gso handlers
net: sched: drop qdisc_reset from dev_graft_qdisc
net: sched: support qdisc_reset on NOLOCK qdisc
net: sched: support skb_bad_tx with lockless qdisc
net: sched: qdisc_qlen for per cpu logic
net: sched: helper to sum qlen
net: sched: lockless support for netif_schedule
net: sched: add support for TCQ_F_NOLOCK subqueues to sch_mq
net: sched: add support for TCQ_F_NOLOCK subqueues to sch_mqprio
net: sched: pfifo_fast use skb_array
include/net/gen_stats.h | 3
include/net/pkt_sched.h | 10 +
include/net/sch_generic.h | 108 +++++++++++
net/core/dev.c | 59 +++++-
net/core/gen_stats.c | 9 +
net/sched/sch_api.c | 21 ++
net/sched/sch_generic.c | 424 ++++++++++++++++++++++++++++++++++-----------
net/sched/sch_mq.c | 25 ++-
net/sched/sch_mqprio.c | 61 ++++--
9 files changed, 567 insertions(+), 153 deletions(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:23:14
Currently __qdisc_run calls qdisc_run_end() but does not call
qdisc_run_begin(). This makes it hard to track pairs of
qdisc_run_{begin,end} across function calls.
To simplify reading these code paths and simpler code this
patch moves begin/end calls into qdisc_run().
Signed-off-by: John Fastabend <redacted>
---
include/net/pkt_sched.h | 4 +++-
net/core/dev.c | 5 +++--
net/sched/sch_generic.c | 2 --
3 files changed, 6 insertions(+), 5 deletions(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:23:41
This patch adds a flag for queueing disciplines to indicate the stack
does not need to use the qdisc lock to protect operations. This can
be used to build lockless scheduling algorithms and improving
performance.
The flag is checked in the tx path and the qdisc lock is only taken
if it is not set. For now use a conditional if statement. Later we
could be more aggressive if it proves worthwhile and use a static key
or wrap this in a likely().
Also the lockless case drops the TCQ_F_CAN_BYPASS logic. The reason
for this is synchronizing a qlen counter across threads proves to
cost more than doing the enqueue/dequeue operations when tested with
pktgen.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 1 +
net/core/dev.c | 26 ++++++++++++++++++++++----
net/sched/sch_generic.c | 24 ++++++++++++++++--------
3 files changed, 39 insertions(+), 12 deletions(-)
@@ -3898,19 +3913,22 @@ static void net_tx_action(struct softirq_action *h)while(head){structQdisc*q=head;-spinlock_t*root_lock;+spinlock_t*root_lock=NULL;head=head->next_sched;-root_lock=qdisc_lock(q);-spin_lock(root_lock);+if(!(q->flags&TCQ_F_NOLOCK)){+root_lock=qdisc_lock(q);+spin_lock(root_lock);+}/* We need to make sure head->next_sched is read*beforeclearing__QDISC_STATE_SCHED*/smp_mb__before_atomic();clear_bit(__QDISC_STATE_SCHED,&q->state);qdisc_run(q);-spin_unlock(root_lock);+if(!(q->flags&TCQ_F_NOLOCK))+spin_unlock(root_lock);}}}
@@ -170,7 +170,8 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,intret=NETDEV_TX_BUSY;/* And release qdisc */-spin_unlock(root_lock);+if(!(q->flags&TCQ_F_NOLOCK))+spin_unlock(root_lock);/* Note that we validate skb (GSO, checksum, ...) outside of locks */if(validate)
@@ -183,10 +184,13 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,HARD_TX_UNLOCK(dev,txq);}else{-spin_lock(root_lock);+if(!(q->flags&TCQ_F_NOLOCK))+spin_lock(root_lock);returnqdisc_qlen(q);}-spin_lock(root_lock);++if(!(q->flags&TCQ_F_NOLOCK))+spin_lock(root_lock);if(dev_xmit_complete(ret)){/* Driver sent out skb successfully or skb was consumed */
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:24:07
sch_direct_xmit() uses qdisc_qlen as a return value but all call sites
of the routine only check if it is zero or not. Simplify the logic so
that we don't need to return an actual queue length value.
This introduces a case now where sch_direct_xmit would have returned
a qlen of zero but now it returns true. However in this case all
call sites of sch_direct_xmit will implement a dequeue() and get
a null skb and abort. This trades tracking qlen in the hotpath for
an extra dequeue operation. Overall this seems to be good for
performance.
Signed-off-by: John Fastabend <redacted>
---
include/net/pkt_sched.h | 6 +++---
net/sched/sch_generic.c | 23 ++++++++++++-----------
2 files changed, 15 insertions(+), 14 deletions(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:24:39
The per cpu qstats support was added with per cpu bstat support which
is currently used by the ingress qdisc. This patch adds a set of
helpers needed to make other qdiscs that use qstats per cpu as well.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:25:12
Enable dflt qdisc support for per cpu stats before this patch a
dflt qdisc was required to use the global statistics qstats and
bstats.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_generic.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
@@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,structQdisc*sch;if(!try_module_get(ops->owner))-gotoerrout;+returnNULL;sch=qdisc_alloc(dev_queue,ops);if(IS_ERR(sch))-gotoerrout;+returnNULL;sch->parent=parentid;-if(!ops->init||ops->init(sch,NULL)==0)+if(!ops->init)returnsch;-qdisc_destroy(sch);+if(ops->init(sch,NULL))+gotoerrout;++/* init() may have set percpu flags so init data structures */+if(qdisc_is_percpu_stats(sch)){+sch->cpu_bstats=+netdev_alloc_pcpu_stats(structgnet_stats_basic_cpu);+if(!sch->cpu_bstats)+gotoerrout;++sch->cpu_qstats=alloc_percpu(structgnet_stats_queue);+if(!sch->cpu_qstats)+gotoerrout;+}++returnsch;errout:+qdisc_destroy(sch);returnNULL;}EXPORT_SYMBOL(qdisc_create_dflt);
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:25:38
The net sched infrastructure has a gso ptr that points to skb structs
that have failed to be enqueued by the device driver.
This can happen when multiple cores try to push a skb onto the same
underlying hardware queue resulting in lock contention. This case is
handled by a cpu collision handler handle_dev_cpu_collision(). Another
case occurs when the stack overruns the drivers low level tx queues
capacity. Ideally these should be a rare occurrence in a well-tuned
system but they do happen.
To handle this in the lockless case use a per cpu gso field to park
the skb until the conflict can be resolved. Note at this point the
skb has already been popped off the qdisc so it has to be handled
by the infrastructure.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 39 +++++++++++++++++++++++++
net/sched/sch_api.c | 7 ++++
net/sched/sch_generic.c | 71 ++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 112 insertions(+), 5 deletions(-)
@@ -717,6 +723,23 @@ static inline struct sk_buff *qdisc_peek_dequeued(struct Qdisc *sch)returnsch->gso_skb;}+staticinlinestructsk_buff*qdisc_peek_dequeued_cpu(structQdisc*sch)+{+structgso_cell*gso=this_cpu_ptr(sch->gso_cpu_skb);++if(!gso->skb){+structsk_buff*skb=sch->dequeue(sch);++if(skb){+gso->skb=skb;+qdisc_qstats_cpu_backlog_inc(sch,skb);+qdisc_qstats_cpu_qlen_inc(sch);+}+}++returngso->skb;+}+/* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */staticinlinestructsk_buff*qdisc_dequeue_peeked(structQdisc*sch){
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:26:03
In qdisc_graft_qdisc a "new" qdisc is attached and the 'qdisc_destroy'
operation is called on the old qdisc. The destroy operation will wait
a rcu grace period and call qdisc_rcu_free(). At which point
gso_cpu_skb is free'd along with all stats so no need to zero stats
and gso_cpu_skb from the graft operation itself.
Further after dropping the qdisc locks we can not continue to call
qdisc_reset before waiting an rcu grace period so that the qdisc is
detached from all cpus. By removing the qdisc_reset() here we get
the correct property of waiting an rcu grace period and letting the
qdisc_destroy operation clean up the qdisc correctly.
Note, a refcnt greater than 1 would cause the destroy operation to
be aborted however if this ever happened the reference to the qdisc
would be lost and we would have a memory leak.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_generic.c | 4 ----
1 file changed, 4 deletions(-)
@@ -813,10 +813,6 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,root_lock=qdisc_lock(oqdisc);spin_lock_bh(root_lock);-/* Prune old scheduler */-if(oqdisc&&atomic_read(&oqdisc->refcnt)<=1)-qdisc_reset(oqdisc);-/* ... and graft new one */if(qdisc==NULL)qdisc=&noop_qdisc;
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:26:28
The qdisc_reset operation depends on the qdisc lock at the moment
to halt any additions to gso_skb and statistics while the list is
free'd and the stats zeroed.
Without the qdisc lock we can not guarantee another cpu is not in
the process of adding a skb to one of the "cells".
To resolve the dev_deactivate sequence that can come from a user
bringing the interface down which causes the gso_skb list to be
flushed and the qlen zero'd. At the moment this is protected by the
qdisc lock so while we clear the qlen/gso_skb fields we are guaranteed
no new skbs are added. For the lockless case though this is not true.
To resolve this move the qdisc_reset call after the new qdisc is
assigned and a grace period is exercised to ensure no new skbs can be
enqueued. Further the RTNL lock is held so we can not get another call
to activate the qdisc while the skb lists are being free'd.
Finally, fix qdisc_reset to handle the per cpu stats and skb lists.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_generic.c | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
@@ -983,20 +1005,27 @@ void dev_deactivate_many(struct list_head *head)&noop_qdisc);dev_watchdog_down(dev);-sync_needed|=!dev->dismantle;}/* Wait for outstanding qdisc-less dev_queue_xmit calls.*Thisisavoidedifalldevicesareindismantlephase:*Callerwillcallsynchronize_net()forus*/-if(sync_needed)-synchronize_net();+synchronize_net();/* Wait for outstanding qdisc_run calls. */-list_for_each_entry(dev,head,close_list)+list_for_each_entry(dev,head,close_list){while(some_qdisc_is_busy(dev))yield();++/* The new qdisc is assigned at this point so we can safely+*unwindstaleskblistsandqdiscstatistics+*/+netdev_for_each_tx_queue(dev,dev_qdisc_reset,NULL);+if(dev_ingress_queue(dev))+dev_qdisc_reset(dev,dev_ingress_queue(dev),NULL);+}+}voiddev_deactivate(structnet_device*dev)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:26:53
Similar to how gso is handled skb_bad_tx needs to be per cpu to handle
lockless qdisc with multiple writer/producers.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 7 +++
net/sched/sch_api.c | 6 +++
net/sched/sch_generic.c | 95 +++++++++++++++++++++++++++++++++++++++++----
3 files changed, 99 insertions(+), 9 deletions(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:27:16
Add qdisc qlen helper routines for lockless qdiscs to use.
The qdisc qlen is no longer used in the hotpath but it is reported
via stats query on the qdisc so it still needs to be tracked. This
adds the per cpu operations needed.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 8 ++++++++
1 file changed, 8 insertions(+)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:27:40
Reporting qlen when qlen is per cpu requires aggregating the per
cpu counters. This adds a helper routine for this.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 15 +++++++++++++++
net/sched/sch_api.c | 3 ++-
2 files changed, 17 insertions(+), 1 deletion(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:28:10
netif_schedule uses a bit QDISC_STATE_SCHED to tell the qdisc layer
if a run of the qdisc has been scheduler. This is important when
tearing down qdisc instances. We can not rcu_free an instance for
example if its possible that we might have outstanding references to
it.
Perhaps more importantly in the per cpu lockless case we need to
schedule a run of the qdisc on all qdiscs that are enqueu'ing packets
and hitting the gso_skb requeue logic or else the skb may get stuck
on the gso_skb queue without anything to finish the xmit.
This patch uses a reference counter instead of a bit to account for
the multiple CPUs.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 1 +
net/core/dev.c | 32 +++++++++++++++++++++++---------
net/sched/sch_api.c | 5 +++++
net/sched/sch_generic.c | 15 ++++++++++++++-
4 files changed, 43 insertions(+), 10 deletions(-)
@@ -3920,15 +3926,23 @@ static void net_tx_action(struct softirq_action *h)if(!(q->flags&TCQ_F_NOLOCK)){root_lock=qdisc_lock(q);spin_lock(root_lock);-}-/* We need to make sure head->next_sched is read-*beforeclearing__QDISC_STATE_SCHED-*/-smp_mb__before_atomic();-clear_bit(__QDISC_STATE_SCHED,&q->state);-qdisc_run(q);-if(!(q->flags&TCQ_F_NOLOCK))++/* We need to make sure head->next_sched is read+*beforeclearing__QDISC_STATE_SCHED+*/+smp_mb__before_atomic();+clear_bit(__QDISC_STATE_SCHED,&q->state);++qdisc_run(q);+spin_unlock(root_lock);+}else{+unsignedlong*s=this_cpu_ptr(q->cpu_state);++smp_mb__before_atomic();+clear_bit(__QDISC_STATE_SCHED,s);+__qdisc_run(q);+}}}}
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:28:28
The sch_mq qdisc creates a sub-qdisc per tx queue which are then
called independently for enqueue and dequeue operations. However
statistics are aggregated and pushed up to the "master" qdisc.
This patch adds support for any of the sub-qdiscs to be per cpu
statistic qdiscs. To handle this case add a check when calculating
stats and aggregate the per cpu stats if needed.
Also exports __gnet_stats_copy_queue() to use as a helper function.
Signed-off-by: John Fastabend <redacted>
---
include/net/gen_stats.h | 3 +++
net/core/gen_stats.c | 9 +++++----
net/sched/sch_mq.c | 25 ++++++++++++++++++-------
3 files changed, 26 insertions(+), 11 deletions(-)
From: Eric Dumazet <hidden> Date: 2016-08-23 20:38:11
On Tue, 2016-08-23 at 13:22 -0700, John Fastabend wrote:
Currently __qdisc_run calls qdisc_run_end() but does not call
qdisc_run_begin(). This makes it hard to track pairs of
qdisc_run_{begin,end} across function calls.
To simplify reading these code paths and simpler code this
patch moves begin/end calls into qdisc_run().
Signed-off-by: John Fastabend <redacted>
---
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:41:59
This converts the pfifo_fast qdisc to use the skb_array data structure
and set the lockless qdisc bit.
This also removes the logic used to pick the next band to dequeue from
and instead just checks a per priority array for packets from top priority
to lowest. This might need to be a bit more clever but seems to work
for now.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_generic.c | 131 +++++++++++++++++++++++++++--------------------
1 file changed, 75 insertions(+), 56 deletions(-)
@@ -654,24 +646,51 @@ nla_put_failure:staticintpfifo_fast_init(structQdisc*qdisc,structnlattr*opt){-intprio;+unsignedintqlen=qdisc_dev(qdisc)->tx_queue_len;structpfifo_fast_priv*priv=qdisc_priv(qdisc);+intprio;++/* guard against zero length rings */+if(!qlen)+return-EINVAL;++for(prio=0;prio<PFIFO_FAST_BANDS;prio++){+structskb_array*q=band2list(priv,prio);+interr;-for(prio=0;prio<PFIFO_FAST_BANDS;prio++)-__skb_queue_head_init(band2list(priv,prio));+err=skb_array_init(q,qlen,GFP_KERNEL);+if(err)+return-ENOMEM;+}/* Can by-pass the queue discipline */qdisc->flags|=TCQ_F_CAN_BYPASS;+qdisc->flags|=TCQ_F_NOLOCK;+qdisc->flags|=TCQ_F_CPUSTATS;+return0;}+staticvoidpfifo_fast_destroy(structQdisc*sch)+{+structpfifo_fast_priv*priv=qdisc_priv(sch);+intprio;++for(prio=0;prio<PFIFO_FAST_BANDS;prio++){+structskb_array*q=band2list(priv,prio);++skb_array_cleanup(q);+}+}+structQdisc_opspfifo_fast_ops__read_mostly={.id="pfifo_fast",.priv_size=sizeof(structpfifo_fast_priv),.enqueue=pfifo_fast_enqueue,.dequeue=pfifo_fast_dequeue,-.peek=pfifo_fast_peek,+.peek=qdisc_peek_dequeued_cpu,.init=pfifo_fast_init,+.destroy=pfifo_fast_destroy,.reset=pfifo_fast_reset,.dump=pfifo_fast_dump,.owner=THIS_MODULE,
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 20:42:30
The sch_mqprio qdisc creates a sub-qdisc per tx queue which are then
called independently for enqueue and dequeue operations. However
statistics are aggregated and pushed up to the "master" qdisc.
This patch adds support for any of the sub-qdiscs to be per cpu
statistic qdiscs. To handle this case add a check when calculating
stats and aggregate the per cpu stats if needed.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_mqprio.c | 61 +++++++++++++++++++++++++++++++-----------------
1 file changed, 39 insertions(+), 22 deletions(-)
From: Eric Dumazet <hidden> Date: 2016-08-23 21:23:18
On Tue, 2016-08-23 at 13:23 -0700, John Fastabend wrote:
sch_direct_xmit() uses qdisc_qlen as a return value but all call sites
of the routine only check if it is zero or not. Simplify the logic so
that we don't need to return an actual queue length value.
This introduces a case now where sch_direct_xmit would have returned
a qlen of zero but now it returns true. However in this case all
call sites of sch_direct_xmit will implement a dequeue() and get
a null skb and abort. This trades tracking qlen in the hotpath for
an extra dequeue operation. Overall this seems to be good for
performance.
Signed-off-by: John Fastabend <redacted>
---
From: Eric Dumazet <hidden> Date: 2016-08-23 21:27:04
On Tue, 2016-08-23 at 13:23 -0700, John Fastabend wrote:
quoted hunk
This patch adds a flag for queueing disciplines to indicate the stack
does not need to use the qdisc lock to protect operations. This can
be used to build lockless scheduling algorithms and improving
performance.
The flag is checked in the tx path and the qdisc lock is only taken
if it is not set. For now use a conditional if statement. Later we
could be more aggressive if it proves worthwhile and use a static key
or wrap this in a likely().
Also the lockless case drops the TCQ_F_CAN_BYPASS logic. The reason
for this is synchronizing a qlen counter across threads proves to
cost more than doing the enqueue/dequeue operations when tested with
pktgen.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 1 +
net/core/dev.c | 26 ++++++++++++++++++++++----
net/sched/sch_generic.c | 24 ++++++++++++++++--------
3 files changed, 39 insertions(+), 12 deletions(-)
@@ -3898,19 +3913,22 @@ static void net_tx_action(struct softirq_action *h)while(head){structQdisc*q=head;-spinlock_t*root_lock;+spinlock_t*root_lock=NULL;head=head->next_sched;-root_lock=qdisc_lock(q);-spin_lock(root_lock);+if(!(q->flags&TCQ_F_NOLOCK)){+root_lock=qdisc_lock(q);+spin_lock(root_lock);+}/* We need to make sure head->next_sched is read*beforeclearing__QDISC_STATE_SCHED*/smp_mb__before_atomic();clear_bit(__QDISC_STATE_SCHED,&q->state);qdisc_run(q);-spin_unlock(root_lock);+if(!(q->flags&TCQ_F_NOLOCK))
This might be faster to use :
if (root_lock) (one less memory read and mask)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-23 22:33:30
On 16-08-23 02:08 PM, Eric Dumazet wrote:
On Tue, 2016-08-23 at 13:23 -0700, John Fastabend wrote:
quoted
This patch adds a flag for queueing disciplines to indicate the
stack does not need to use the qdisc lock to protect operations.
This can be used to build lockless scheduling algorithms and
improving performance.
[...]
quoted
* Heuristic to force contended enqueues to serialize on a *
separate lock before trying to get qdisc main lock. @@ -3898,19
+3913,22 @@ static void net_tx_action(struct softirq_action *h)
while (head) { struct Qdisc *q = head; - spinlock_t *root_lock; +
spinlock_t *root_lock = NULL;
head = head->next_sched;
- root_lock = qdisc_lock(q); - spin_lock(root_lock); + if
(!(q->flags & TCQ_F_NOLOCK)) { + root_lock = qdisc_lock(q); +
spin_lock(root_lock); + } /* We need to make sure
head->next_sched is read * before clearing __QDISC_STATE_SCHED */
smp_mb__before_atomic(); clear_bit(__QDISC_STATE_SCHED,
&q->state); qdisc_run(q); - spin_unlock(root_lock); + if
(!(q->flags & TCQ_F_NOLOCK))
This might be faster to use : if (root_lock) (one less memory read
and mask)
hmm this actually gets factored out in patch 12 but I'll go ahead
and make this change and then I think it reads a bit better through
the series.
quoted
+ spin_unlock(root_lock); } } } diff --git
a/net/sched/sch_generic.c b/net/sched/sch_generic.c index
e305a55..af32418 100644 --- a/net/sched/sch_generic.c +++
b/net/sched/sch_generic.c @@ -170,7 +170,8 @@ int
sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, int ret =
NETDEV_TX_BUSY;
/* And release qdisc */ - spin_unlock(root_lock); + if (!(q->flags
& TCQ_F_NOLOCK)) + spin_unlock(root_lock);
You might use the same trick, if root_lock is NULL for lockless
qdisc.
So what I just did is pass NULL into sch_direct_xmit() for root_lock
when the qdisc is lockless. This replaces the qdisc flags checks in this
call to checking root_lock.
Seems like a nice cleanup/optimization. I'll wait a bit and then push
it in v2 after giving folks a day or two to review this set.
From: Eric Dumazet <hidden> Date: 2016-08-23 23:26:12
On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote:
quoted hunk
The per cpu qstats support was added with per cpu bstat support which
is currently used by the ingress qdisc. This patch adds a set of
helpers needed to make other qdiscs that use qstats per cpu as well.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-24 00:06:22
On 16-08-23 04:25 PM, Eric Dumazet wrote:
On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote:
quoted
The per cpu qstats support was added with per cpu bstat support which
is currently used by the ingress qdisc. This patch adds a set of
helpers needed to make other qdiscs that use qstats per cpu as well.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
From: Eric Dumazet <hidden> Date: 2016-08-24 16:30:00
On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote:
quoted hunk
Enable dflt qdisc support for per cpu stats before this patch a
dflt qdisc was required to use the global statistics qstats and
bstats.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_generic.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
@@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,structQdisc*sch;if(!try_module_get(ops->owner))-gotoerrout;+returnNULL;sch=qdisc_alloc(dev_queue,ops);if(IS_ERR(sch))-gotoerrout;+returnNULL;sch->parent=parentid;-if(!ops->init||ops->init(sch,NULL)==0)+if(!ops->init)returnsch;-qdisc_destroy(sch);+if(ops->init(sch,NULL))+gotoerrout;++/* init() may have set percpu flags so init data structures */+if(qdisc_is_percpu_stats(sch)){+sch->cpu_bstats=+netdev_alloc_pcpu_stats(structgnet_stats_basic_cpu);+if(!sch->cpu_bstats)+gotoerrout;++sch->cpu_qstats=alloc_percpu(structgnet_stats_queue);+if(!sch->cpu_qstats)+gotoerrout;+}++returnsch;errout:+qdisc_destroy(sch);returnNULL;}EXPORT_SYMBOL(qdisc_create_dflt);
Hmm... apparently we have bug here, added in
6da7c8fcbcbdb50ec ("qdisc: allow setting default queuing discipline")
We do not undo the try_module_get() in case of an error.
I will send a fix.
From: Eric Dumazet <hidden> Date: 2016-08-24 16:41:50
On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote:
quoted hunk
Enable dflt qdisc support for per cpu stats before this patch a
dflt qdisc was required to use the global statistics qstats and
bstats.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_generic.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
@@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,structQdisc*sch;if(!try_module_get(ops->owner))-gotoerrout;+returnNULL;sch=qdisc_alloc(dev_queue,ops);if(IS_ERR(sch))-gotoerrout;+returnNULL;sch->parent=parentid;-if(!ops->init||ops->init(sch,NULL)==0)+if(!ops->init)returnsch;-qdisc_destroy(sch);+if(ops->init(sch,NULL))+gotoerrout;++/* init() may have set percpu flags so init data structures */+if(qdisc_is_percpu_stats(sch)){+sch->cpu_bstats=+netdev_alloc_pcpu_stats(structgnet_stats_basic_cpu);+if(!sch->cpu_bstats)+gotoerrout;++sch->cpu_qstats=alloc_percpu(structgnet_stats_queue);+if(!sch->cpu_qstats)+gotoerrout;+}+
Why are you attempting these allocations here instead of qdisc_alloc()
This looks weird, I would expect base qdisc being fully allocated before
ops->init() is attempted.
From: Eric Dumazet <hidden> Date: 2016-08-24 17:26:58
On Wed, 2016-08-24 at 10:13 -0700, John Fastabend wrote:
quoted
I could fully allocate it in qdisc_alloc() but we don't know if the
qdisc needs per cpu data structures until after the init call
Should not we have a flag to advertise the need of per spu stats on
qdisc ?
This is not clear why ->init() can know this, and not its caller.
. So it
would sit unused in those cases if done from qdisc_alloc(). It seems
best to me at least to just avoid the allocation in qdisc_alloc() and
do it after init like I did here.
Perhaps it would be nice to pull these into a function call
post_init_qdisc_alloc() that does all this allocation?
.John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-24 17:50:29
On 16-08-24 10:26 AM, Eric Dumazet wrote:
On Wed, 2016-08-24 at 10:13 -0700, John Fastabend wrote:
quoted
quoted
I could fully allocate it in qdisc_alloc() but we don't know if the
qdisc needs per cpu data structures until after the init call
Should not we have a flag to advertise the need of per spu stats on
qdisc ?
This is not clear why ->init() can know this, and not its caller.
sure we could be a static_flags field in the ops structure. What do
you think about doing that?
We would still need some flags to be set at init though like the bypass
bit it looks like some qdiscs set that based on user input.
quoted
. So it
would sit unused in those cases if done from qdisc_alloc(). It seems
best to me at least to just avoid the allocation in qdisc_alloc() and
do it after init like I did here.
Perhaps it would be nice to pull these into a function call
post_init_qdisc_alloc() that does all this allocation?
.John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-24 18:05:45
On 16-08-24 09:41 AM, Eric Dumazet wrote:
On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote:
quoted
Enable dflt qdisc support for per cpu stats before this patch a
dflt qdisc was required to use the global statistics qstats and
bstats.
Signed-off-by: John Fastabend <redacted>
---
net/sched/sch_generic.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
@@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,structQdisc*sch;if(!try_module_get(ops->owner))-gotoerrout;+returnNULL;sch=qdisc_alloc(dev_queue,ops);if(IS_ERR(sch))-gotoerrout;+returnNULL;sch->parent=parentid;-if(!ops->init||ops->init(sch,NULL)==0)+if(!ops->init)returnsch;-qdisc_destroy(sch);+if(ops->init(sch,NULL))+gotoerrout;++/* init() may have set percpu flags so init data structures */+if(qdisc_is_percpu_stats(sch)){+sch->cpu_bstats=+netdev_alloc_pcpu_stats(structgnet_stats_basic_cpu);+if(!sch->cpu_bstats)+gotoerrout;++sch->cpu_qstats=alloc_percpu(structgnet_stats_queue);+if(!sch->cpu_qstats)+gotoerrout;+}+
Why are you attempting these allocations here instead of qdisc_alloc()
This looks weird, I would expect base qdisc being fully allocated before
ops->init() is attempted.
I could fully allocate it in qdisc_alloc() but we don't know if the
qdisc needs per cpu data structures until after the init call. So it
would sit unused in those cases if done from qdisc_alloc(). It seems
best to me at least to just avoid the allocation in qdisc_alloc() and
do it after init like I did here.
Perhaps it would be nice to pull these into a function call
post_init_qdisc_alloc() that does all this allocation?
.John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-09-07 14:51:08
On 16-08-23 01:27 PM, John Fastabend wrote:
quoted hunk
netif_schedule uses a bit QDISC_STATE_SCHED to tell the qdisc layer
if a run of the qdisc has been scheduler. This is important when
tearing down qdisc instances. We can not rcu_free an instance for
example if its possible that we might have outstanding references to
it.
Perhaps more importantly in the per cpu lockless case we need to
schedule a run of the qdisc on all qdiscs that are enqueu'ing packets
and hitting the gso_skb requeue logic or else the skb may get stuck
on the gso_skb queue without anything to finish the xmit.
This patch uses a reference counter instead of a bit to account for
the multiple CPUs.
Signed-off-by: John Fastabend <redacted>
---
include/net/sch_generic.h | 1 +
net/core/dev.c | 32 +++++++++++++++++++++++---------
net/sched/sch_api.c | 5 +++++
net/sched/sch_generic.c | 15 ++++++++++++++-
4 files changed, 43 insertions(+), 10 deletions(-)
I've had this lingering issue with this patchset that I think I've
pinned down. The issue manifests itself as a hang every once and awhile
when adding a new qdisc where the qdisc add code is stuck waiting for
the QDISC_STATE_SCHED bit to be zeroed.
The base issue is __netif_schedule gets called from other contexts
other than the enqueue()/dequeue() paths. In the dequeue() path if a
packet can not be pushed at the driver with this series it will park
the skb on one of the gso or bad pkt per cpu slots and signal
__netif_schedule to retry. This works because the skb is on the per
cpu slot of the running cpu and the __netif_schedule is signaled on
the running cpu so everything aligns.
However if the txq is frozen_or_stopped by the driver the dequeue
path in the qdisc layer just aborts and waits for the driver to signal
it to wake up via a netif_schedule call. But there is no guarantee that
the driver submitted netif_schedule call is going to come back on the
"right" per cpu. Nor does the soft_irq structure have any way to signal
an interrupt on a specific cpu. So if the driver signals a soft_irq on
some other core than we happily process it but there is nothing to kick
the qdisc stack to dequeue packets on other cores.
TXQs tend to get frozen or stopped (netif_xmit_frozen_or_stopped) via
BQL or if you have flow control on which is how I got this to hang more
reliably.
I'm looking at coming up with some sort of fix now but not entirely
sure the best mechanism at the moment. One possibility is to not backoff
from the qdisc layer in the case of frozen_or_stopped queues and
believe that the txq really shouldn't be frozen_or_stopped for very
long. Another is to signal a soft_irq on any cpu core with packets which
we can "learn" via per cpu qlen values.
Any other ideas would be welcome. Just thought I would point out why
this series was taking so long to get a v2 out. Also behind on reviewing
other patches....
Thanks,
John