* [PATCH net-next 1/5] net: caif: avoid using qdisc_qlen()
2019-04-08 16:35 [PATCH net-next 0/5] net: sched: move back qlen to per CPU accounting Paolo Abeni
@ 2019-04-08 16:35 ` Paolo Abeni
2019-04-08 16:35 ` [PATCH net-next 2/5] net: sched: prefer qdisc_is_empty() over direct qlen access Paolo Abeni
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Paolo Abeni @ 2019-04-08 16:35 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Eric Dumazet, Ivan Vecera
Such helper does not cope correctly with NOLOCK qdiscs.
In the following patches we will move back qlen to per CPU
values for such qdiscs, so qdisc_qlen_sum() is not an option,
too.
Instead, use qlen only for lock qdiscs, and always set
flow off for NOLOCK qdiscs with a not empty tx queue.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/caif/caif_dev.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index 711d7156efd8..6c6e01963aac 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -186,15 +186,19 @@ static int transmit(struct cflayer *layer, struct cfpkt *pkt)
goto noxoff;
if (likely(!netif_queue_stopped(caifd->netdev))) {
+ struct Qdisc *sch;
+
/* If we run with a TX queue, check if the queue is too long*/
txq = netdev_get_tx_queue(skb->dev, 0);
- qlen = qdisc_qlen(rcu_dereference_bh(txq->qdisc));
-
- if (likely(qlen == 0))
+ sch = rcu_dereference_bh(txq->qdisc);
+ if (likely(qdisc_is_empty(sch)))
goto noxoff;
+ /* can check for explicit qdisc len value only !NOLOCK,
+ * always set flow off otherwise
+ */
high = (caifd->netdev->tx_queue_len * q_high) / 100;
- if (likely(qlen < high))
+ if (!(sch->flags & TCQ_F_NOLOCK) && likely(sch->q.qlen < high))
goto noxoff;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 2/5] net: sched: prefer qdisc_is_empty() over direct qlen access
2019-04-08 16:35 [PATCH net-next 0/5] net: sched: move back qlen to per CPU accounting Paolo Abeni
2019-04-08 16:35 ` [PATCH net-next 1/5] net: caif: avoid using qdisc_qlen() Paolo Abeni
@ 2019-04-08 16:35 ` Paolo Abeni
2019-04-08 16:35 ` [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS Paolo Abeni
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Paolo Abeni @ 2019-04-08 16:35 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Eric Dumazet, Ivan Vecera
When checking for root qdisc queue length, do not access directly q.qlen.
In the following patches we will move back qlen accounting to per CPU
values for NOLOCK qdiscs.
Instead, prefer the qdisc_is_empty() helper usage.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/sch_generic.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 83364fac7cf1..19206d669655 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -747,7 +747,7 @@ static inline bool qdisc_all_tx_empty(const struct net_device *dev)
struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
const struct Qdisc *q = rcu_dereference(txq->qdisc);
- if (q->q.qlen) {
+ if (!qdisc_is_empty(q)) {
rcu_read_unlock();
return false;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS
2019-04-08 16:35 [PATCH net-next 0/5] net: sched: move back qlen to per CPU accounting Paolo Abeni
2019-04-08 16:35 ` [PATCH net-next 1/5] net: caif: avoid using qdisc_qlen() Paolo Abeni
2019-04-08 16:35 ` [PATCH net-next 2/5] net: sched: prefer qdisc_is_empty() over direct qlen access Paolo Abeni
@ 2019-04-08 16:35 ` Paolo Abeni
2019-04-09 9:50 ` kbuild test robot
2019-04-09 10:19 ` kbuild test robot
2019-04-08 16:35 ` [PATCH net-next 4/5] net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too Paolo Abeni
2019-04-08 16:35 ` [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location" Paolo Abeni
4 siblings, 2 replies; 13+ messages in thread
From: Paolo Abeni @ 2019-04-08 16:35 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Eric Dumazet, Ivan Vecera
The core sched implementation checks independently for NOLOCK flag
to acquire/release the root spin lock and for qdisc_is_percpu_stats()
to account per CPU values in many places.
This change update the last few places checking the TCQ_F_NOLOCK to
do per CPU stats accounting according to qdisc_is_percpu_stats()
value.
The above allows to clean dev_requeue_skb() implementation a bit
and makes stats update always consistent with a single flag.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/sch_generic.h | 25 ++++++++++++--------
net/sched/sch_generic.c | 50 +++++++++++++--------------------------
2 files changed, 32 insertions(+), 43 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 19206d669655..249817d97bf7 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -146,13 +146,6 @@ static inline bool qdisc_is_running(struct Qdisc *qdisc)
return (raw_read_seqcount(&qdisc->running) & 1) ? true : false;
}
-static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
-{
- if (qdisc->flags & TCQ_F_NOLOCK)
- return qdisc->empty;
- return !qdisc->q.qlen;
-}
-
static inline bool qdisc_run_begin(struct Qdisc *qdisc)
{
if (qdisc->flags & TCQ_F_NOLOCK) {
@@ -490,7 +483,7 @@ static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
{
u32 qlen = q->qstats.qlen;
- if (q->flags & TCQ_F_NOLOCK)
+ if (q->flags & TCQ_F_CPUSTATS)
qlen += atomic_read(&q->q.atomic_qlen);
else
qlen += q->q.qlen;
@@ -822,6 +815,13 @@ static inline bool qdisc_is_percpu_stats(const struct Qdisc *q)
return q->flags & TCQ_F_CPUSTATS;
}
+static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
+{
+ if (qdisc_is_percpu_stats(qdisc))
+ return qdisc->empty;
+ return !qdisc->q.qlen;
+}
+
static inline void _bstats_update(struct gnet_stats_basic_packed *bstats,
__u64 bytes, __u32 packets)
{
@@ -1113,8 +1113,13 @@ static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
if (skb) {
skb = __skb_dequeue(&sch->gso_skb);
- qdisc_qstats_backlog_dec(sch, skb);
- sch->q.qlen--;
+ if (qdisc_is_percpu_stats(sch)) {
+ qdisc_qstats_cpu_backlog_dec(sch, skb);
+ qdisc_qstats_atomic_qlen_dec(sch);
+ } else {
+ qdisc_qstats_backlog_dec(sch, skb);
+ sch->q.qlen--;
+ }
} else {
skb = sch->dequeue(sch);
}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 81356ef38d1d..ddff2952be87 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -118,52 +118,36 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
spin_unlock(lock);
}
-static inline int __dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
+static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
{
- while (skb) {
- struct sk_buff *next = skb->next;
-
- __skb_queue_tail(&q->gso_skb, skb);
- q->qstats.requeues++;
- qdisc_qstats_backlog_inc(q, skb);
- q->q.qlen++; /* it's still part of the queue */
+ spinlock_t *lock = NULL;
- skb = next;
+ if (q->flags & TCQ_F_NOLOCK) {
+ lock = qdisc_lock(q);
+ spin_lock(lock);
}
- __netif_schedule(q);
-
- return 0;
-}
-static inline int dev_requeue_skb_locked(struct sk_buff *skb, struct Qdisc *q)
-{
- spinlock_t *lock = qdisc_lock(q);
-
- spin_lock(lock);
while (skb) {
struct sk_buff *next = skb->next;
__skb_queue_tail(&q->gso_skb, skb);
- qdisc_qstats_cpu_requeues_inc(q);
- qdisc_qstats_cpu_backlog_inc(q, skb);
- qdisc_qstats_atomic_qlen_inc(q);
+ /* it's still part of the queue */
+ if (qdisc_is_percpu_stats(q)) {
+ qdisc_qstats_cpu_requeues_inc(q);
+ qdisc_qstats_cpu_backlog_inc(q, skb);
+ qdisc_qstats_atomic_qlen_inc(q);
+ } else {
+ q->qstats.requeues++;
+ qdisc_qstats_backlog_inc(q, skb);
+ q->q.qlen++;
+ }
skb = next;
}
- spin_unlock(lock);
-
+ if (lock)
+ spin_unlock(lock);
__netif_schedule(q);
-
- return 0;
-}
-
-static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
-{
- if (q->flags & TCQ_F_NOLOCK)
- return dev_requeue_skb_locked(skb, q);
- else
- return __dev_requeue_skb(skb, q);
}
static void try_bulk_dequeue_skb(struct Qdisc *q,
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS
2019-04-08 16:35 ` [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS Paolo Abeni
@ 2019-04-09 9:50 ` kbuild test robot
2019-04-09 10:41 ` Paolo Abeni
2019-04-09 10:19 ` kbuild test robot
1 sibling, 1 reply; 13+ messages in thread
From: kbuild test robot @ 2019-04-09 9:50 UTC (permalink / raw)
To: Paolo Abeni
Cc: kbuild-all, netdev, David S. Miller, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, Eric Dumazet, Ivan Vecera
[-- Attachment #1: Type: text/plain, Size: 9347 bytes --]
Hi Paolo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: i386-randconfig-x077-201914 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from net//vmw_vsock/af_vsock.c:118:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
>> include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
ida_is_empty
include/net/sch_generic.h: At top level:
>> include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^~~~~~~~~~~~~~
include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from include/net/inet_sock.h:26,
from include/net/inet_connection_sock.h:24,
from include/linux/dccp.h:13,
from net//dccp/proto.c:12:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
>> include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
ida_is_empty
include/net/sch_generic.h: At top level:
>> include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^~~~~~~~~~~~~~
include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
In file included from net//dccp/trace.h:84:0,
from net//dccp/proto.c:42:
include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
^
cc1: some warnings being treated as errors
compilation terminated.
vim +743 include/net/sch_generic.h
5aa709954 David S. Miller 2008-07-08 732
3e745dd69 David S. Miller 2008-07-08 733 /* Are all TX queues of the device empty? */
3e745dd69 David S. Miller 2008-07-08 734 static inline bool qdisc_all_tx_empty(const struct net_device *dev)
3e745dd69 David S. Miller 2008-07-08 735 {
e8a0464cc David S. Miller 2008-07-17 736 unsigned int i;
46e5da40a John Fastabend 2014-09-12 737
46e5da40a John Fastabend 2014-09-12 738 rcu_read_lock();
e8a0464cc David S. Miller 2008-07-17 739 for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17 740 struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend 2014-09-12 741 const struct Qdisc *q = rcu_dereference(txq->qdisc);
3e745dd69 David S. Miller 2008-07-08 742
630dee66e Paolo Abeni 2019-04-08 @743 if (!qdisc_is_empty(q)) {
46e5da40a John Fastabend 2014-09-12 744 rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17 745 return false;
e8a0464cc David S. Miller 2008-07-17 746 }
46e5da40a John Fastabend 2014-09-12 747 }
46e5da40a John Fastabend 2014-09-12 748 rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17 749 return true;
3e745dd69 David S. Miller 2008-07-08 750 }
3e745dd69 David S. Miller 2008-07-08 751
6fa9864b5 David S. Miller 2008-07-08 752 /* Are any of the TX qdiscs changing? */
05bdd2f14 Eric Dumazet 2011-10-20 753 static inline bool qdisc_tx_changing(const struct net_device *dev)
6fa9864b5 David S. Miller 2008-07-08 754 {
e8a0464cc David S. Miller 2008-07-17 755 unsigned int i;
46e5da40a John Fastabend 2014-09-12 756
e8a0464cc David S. Miller 2008-07-17 757 for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17 758 struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend 2014-09-12 759 if (rcu_access_pointer(txq->qdisc) != txq->qdisc_sleeping)
e8a0464cc David S. Miller 2008-07-17 760 return true;
e8a0464cc David S. Miller 2008-07-17 761 }
e8a0464cc David S. Miller 2008-07-17 762 return false;
6fa9864b5 David S. Miller 2008-07-08 763 }
6fa9864b5 David S. Miller 2008-07-08 764
e8a0464cc David S. Miller 2008-07-17 765 /* Is the device using the noop qdisc on all queues? */
052979499 David S. Miller 2008-07-08 766 static inline bool qdisc_tx_is_noop(const struct net_device *dev)
052979499 David S. Miller 2008-07-08 767 {
e8a0464cc David S. Miller 2008-07-17 768 unsigned int i;
46e5da40a John Fastabend 2014-09-12 769
e8a0464cc David S. Miller 2008-07-17 770 for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17 771 struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend 2014-09-12 772 if (rcu_access_pointer(txq->qdisc) != &noop_qdisc)
e8a0464cc David S. Miller 2008-07-17 773 return false;
e8a0464cc David S. Miller 2008-07-17 774 }
e8a0464cc David S. Miller 2008-07-17 775 return true;
052979499 David S. Miller 2008-07-08 776 }
052979499 David S. Miller 2008-07-08 777
bfe0d0298 Eric Dumazet 2011-01-09 778 static inline unsigned int qdisc_pkt_len(const struct sk_buff *skb)
0abf77e55 Jussi Kivilinna 2008-07-20 779 {
175f9c1bb Jussi Kivilinna 2008-07-20 780 return qdisc_skb_cb(skb)->pkt_len;
0abf77e55 Jussi Kivilinna 2008-07-20 781 }
0abf77e55 Jussi Kivilinna 2008-07-20 782
c27f339af Jarek Poplawski 2008-08-04 783 /* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */
378a2f090 Jarek Poplawski 2008-08-04 784 enum net_xmit_qdisc_t {
378a2f090 Jarek Poplawski 2008-08-04 785 __NET_XMIT_STOLEN = 0x00010000,
c27f339af Jarek Poplawski 2008-08-04 786 __NET_XMIT_BYPASS = 0x00020000,
378a2f090 Jarek Poplawski 2008-08-04 787 };
378a2f090 Jarek Poplawski 2008-08-04 788
c27f339af Jarek Poplawski 2008-08-04 789 #ifdef CONFIG_NET_CLS_ACT
378a2f090 Jarek Poplawski 2008-08-04 790 #define net_xmit_drop_count(e) ((e) & __NET_XMIT_STOLEN ? 0 : 1)
378a2f090 Jarek Poplawski 2008-08-04 791 #else
378a2f090 Jarek Poplawski 2008-08-04 792 #define net_xmit_drop_count(e) (1)
378a2f090 Jarek Poplawski 2008-08-04 793 #endif
378a2f090 Jarek Poplawski 2008-08-04 794
a2da570d6 Eric Dumazet 2011-01-20 795 static inline void qdisc_calculate_pkt_len(struct sk_buff *skb,
a2da570d6 Eric Dumazet 2011-01-20 796 const struct Qdisc *sch)
5f86173bd Jussi Kivilinna 2008-07-20 797 {
3a682fbd7 David S. Miller 2008-07-20 798 #ifdef CONFIG_NET_SCHED
a2da570d6 Eric Dumazet 2011-01-20 799 struct qdisc_size_table *stab = rcu_dereference_bh(sch->stab);
a2da570d6 Eric Dumazet 2011-01-20 800
a2da570d6 Eric Dumazet 2011-01-20 801 if (stab)
a2da570d6 Eric Dumazet 2011-01-20 802 __qdisc_calculate_pkt_len(skb, stab);
3a682fbd7 David S. Miller 2008-07-20 803 #endif
a2da570d6 Eric Dumazet 2011-01-20 804 }
a2da570d6 Eric Dumazet 2011-01-20 805
520ac30f4 Eric Dumazet 2016-06-21 806 static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
520ac30f4 Eric Dumazet 2016-06-21 807 struct sk_buff **to_free)
a2da570d6 Eric Dumazet 2011-01-20 808 {
a2da570d6 Eric Dumazet 2011-01-20 809 qdisc_calculate_pkt_len(skb, sch);
520ac30f4 Eric Dumazet 2016-06-21 810 return sch->enqueue(skb, sch, to_free);
5f86173bd Jussi Kivilinna 2008-07-20 811 }
5f86173bd Jussi Kivilinna 2008-07-20 812
22e0f8b93 John Fastabend 2014-09-28 813 static inline bool qdisc_is_percpu_stats(const struct Qdisc *q)
22e0f8b93 John Fastabend 2014-09-28 814 {
22e0f8b93 John Fastabend 2014-09-28 815 return q->flags & TCQ_F_CPUSTATS;
22e0f8b93 John Fastabend 2014-09-28 816 }
bfe0d0298 Eric Dumazet 2011-01-09 817
9cda4ff7e Paolo Abeni 2019-04-08 @818 static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
9cda4ff7e Paolo Abeni 2019-04-08 819 {
9cda4ff7e Paolo Abeni 2019-04-08 820 if (qdisc_is_percpu_stats(qdisc))
9cda4ff7e Paolo Abeni 2019-04-08 821 return qdisc->empty;
9cda4ff7e Paolo Abeni 2019-04-08 822 return !qdisc->q.qlen;
9cda4ff7e Paolo Abeni 2019-04-08 823 }
9cda4ff7e Paolo Abeni 2019-04-08 824
:::::: The code at line 743 was first introduced by commit
:::::: 630dee66e06a4ec68b87f0d1a9959b54b42f8959 net: sched: prefer qdisc_is_empty() over direct qlen access
:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32236 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS
2019-04-09 9:50 ` kbuild test robot
@ 2019-04-09 10:41 ` Paolo Abeni
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Abeni @ 2019-04-09 10:41 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, netdev, David S. Miller, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, Eric Dumazet, Ivan Vecera
On Tue, 2019-04-09 at 17:50 +0800, kbuild test robot wrote:
> Hi Paolo,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on net-next/master]
>
> url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
> config: i386-randconfig-x077-201914 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
I apologize, a last bunch of local changes did not land into the
submitted patches due to PEBKAC.
(qdisc_is_empty() declaration in the correct place and full replacement
of qdisc_qstats_atomic_qlen_dec in patch 5/5).
I'll fix in v2,
Thanks,
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS
2019-04-08 16:35 ` [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS Paolo Abeni
2019-04-09 9:50 ` kbuild test robot
@ 2019-04-09 10:19 ` kbuild test robot
1 sibling, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-04-09 10:19 UTC (permalink / raw)
To: Paolo Abeni
Cc: kbuild-all, netdev, David S. Miller, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, Eric Dumazet, Ivan Vecera
[-- Attachment #1: Type: text/plain, Size: 6834 bytes --]
Hi Paolo,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: x86_64-randconfig-x001-201914 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:5:0,
from arch/x86/include/asm/bug.h:83,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from net/socket.c:61:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
if (!qdisc_is_empty(q)) {
^~
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from include/net/cls_cgroup.h:19,
from net/socket.c:99:
include/net/sch_generic.h: At top level:
include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^~~~~~~~~~~~~~
In file included from include/asm-generic/bug.h:5:0,
from arch/x86/include/asm/bug.h:83,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from net/socket.c:61:
include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
if (!qdisc_is_empty(q)) {
^~
cc1: some warnings being treated as errors
--
In file included from include/linux/kernel.h:11:0,
from include/linux/uio.h:12,
from include/linux/socket.h:8,
from include/uapi/linux/in.h:24,
from include/linux/in.h:23,
from include/linux/dccp.h:6,
from net//dccp/proto.c:12:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
if (!qdisc_is_empty(q)) {
^~
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from include/net/inet_sock.h:26,
from include/net/inet_connection_sock.h:24,
from include/linux/dccp.h:13,
from net//dccp/proto.c:12:
include/net/sch_generic.h: At top level:
include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^~~~~~~~~~~~~~
In file included from include/linux/kernel.h:11:0,
from include/linux/uio.h:12,
from include/linux/socket.h:8,
from include/uapi/linux/in.h:24,
from include/linux/in.h:23,
from include/linux/dccp.h:6,
from net//dccp/proto.c:12:
include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
if (!qdisc_is_empty(q)) {
^~
In file included from net//dccp/trace.h:84:0,
from net//dccp/proto.c:42:
include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
^
cc1: some warnings being treated as errors
compilation terminated.
vim +/if +743 include/net/sch_generic.h
5aa709954 David S. Miller 2008-07-08 732
3e745dd69 David S. Miller 2008-07-08 733 /* Are all TX queues of the device empty? */
3e745dd69 David S. Miller 2008-07-08 734 static inline bool qdisc_all_tx_empty(const struct net_device *dev)
3e745dd69 David S. Miller 2008-07-08 735 {
e8a0464cc David S. Miller 2008-07-17 736 unsigned int i;
46e5da40a John Fastabend 2014-09-12 737
46e5da40a John Fastabend 2014-09-12 738 rcu_read_lock();
e8a0464cc David S. Miller 2008-07-17 739 for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17 740 struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend 2014-09-12 741 const struct Qdisc *q = rcu_dereference(txq->qdisc);
3e745dd69 David S. Miller 2008-07-08 742
630dee66e Paolo Abeni 2019-04-08 @743 if (!qdisc_is_empty(q)) {
46e5da40a John Fastabend 2014-09-12 744 rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17 745 return false;
e8a0464cc David S. Miller 2008-07-17 746 }
46e5da40a John Fastabend 2014-09-12 747 }
46e5da40a John Fastabend 2014-09-12 748 rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17 749 return true;
3e745dd69 David S. Miller 2008-07-08 750 }
3e745dd69 David S. Miller 2008-07-08 751
:::::: The code at line 743 was first introduced by commit
:::::: 630dee66e06a4ec68b87f0d1a9959b54b42f8959 net: sched: prefer qdisc_is_empty() over direct qlen access
:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32103 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 4/5] net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too
2019-04-08 16:35 [PATCH net-next 0/5] net: sched: move back qlen to per CPU accounting Paolo Abeni
` (2 preceding siblings ...)
2019-04-08 16:35 ` [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS Paolo Abeni
@ 2019-04-08 16:35 ` Paolo Abeni
2019-04-08 16:35 ` [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location" Paolo Abeni
4 siblings, 0 replies; 13+ messages in thread
From: Paolo Abeni @ 2019-04-08 16:35 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Eric Dumazet, Ivan Vecera
Since stats updating is always consistent with TCQ_F_CPUSTATS flag,
we can disable it at qdisc creation time flipping such bit.
In my experiments, if the NOLOCK flag is cleared, per CPU stats
accounting does not give any measurable performance gain, but it
waste some memory.
Let's clear TCQ_F_CPUSTATS together with NOLOCK, when enslaving
a NOLOCK qdisc to 'lock' one.
Use stats update helper inside pfifo_fast, to cope correctly with
TCQ_F_CPUSTATS flag change.
As a side effect, q.qlen value for any child qdiscs is always
consistent for all lock classfull qdiscs.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/sch_generic.h | 26 ++++++++++++++++++++++++++
net/sched/sch_api.c | 15 ++++++++++++++-
net/sched/sch_generic.c | 10 ++--------
3 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 249817d97bf7..cdd1671bdd22 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1106,6 +1106,32 @@ static inline struct sk_buff *qdisc_peek_dequeued(struct Qdisc *sch)
return skb;
}
+static inline void qdisc_update_stats_at_dequeue(struct Qdisc *sch,
+ struct sk_buff *skb)
+{
+ if (qdisc_is_percpu_stats(sch)) {
+ qdisc_qstats_cpu_backlog_dec(sch, skb);
+ qdisc_bstats_cpu_update(sch, skb);
+ qdisc_qstats_atomic_qlen_dec(sch);
+ } else {
+ qdisc_qstats_backlog_dec(sch, skb);
+ qdisc_bstats_update(sch, skb);
+ sch->q.qlen--;
+ }
+}
+
+static inline void qdisc_update_stats_at_enqueue(struct Qdisc *sch,
+ unsigned pkt_len)
+{
+ if (qdisc_is_percpu_stats(sch)) {
+ qdisc_qstats_atomic_qlen_inc(sch);
+ this_cpu_add(sch->cpu_qstats->backlog, pkt_len);
+ } else {
+ sch->qstats.backlog += pkt_len;
+ sch->q.qlen++;
+ }
+}
+
/* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
{
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index fb8f138b9776..c126b9f78d6e 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -998,6 +998,19 @@ static void notify_and_destroy(struct net *net, struct sk_buff *skb,
qdisc_put(old);
}
+static void qdisc_clear_nolock(struct Qdisc *sch)
+{
+ sch->flags &= ~TCQ_F_NOLOCK;
+ if (!(sch->flags & TCQ_F_CPUSTATS))
+ return;
+
+ free_percpu(sch->cpu_bstats);
+ free_percpu(sch->cpu_qstats);
+ sch->cpu_bstats = NULL;
+ sch->cpu_qstats = NULL;
+ sch->flags &= ~TCQ_F_CPUSTATS;
+}
+
/* Graft qdisc "new" to class "classid" of qdisc "parent" or
* to device "dev".
*
@@ -1076,7 +1089,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
/* Only support running class lockless if parent is lockless */
if (new && (new->flags & TCQ_F_NOLOCK) &&
parent && !(parent->flags & TCQ_F_NOLOCK))
- new->flags &= ~TCQ_F_NOLOCK;
+ qdisc_clear_nolock(new);
if (!cops || !cops->graft)
return -EOPNOTSUPP;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ddff2952be87..12a6e1a39fa0 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -629,11 +629,7 @@ static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc *qdisc,
if (unlikely(err))
return qdisc_drop_cpu(skb, qdisc, to_free);
- qdisc_qstats_atomic_qlen_inc(qdisc);
- /* Note: skb can not be used after skb_array_produce(),
- * so we better not use qdisc_qstats_cpu_backlog_inc()
- */
- this_cpu_add(qdisc->cpu_qstats->backlog, pkt_len);
+ qdisc_update_stats_at_enqueue(qdisc, pkt_len);
return NET_XMIT_SUCCESS;
}
@@ -652,9 +648,7 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
skb = __skb_array_consume(q);
}
if (likely(skb)) {
- qdisc_qstats_cpu_backlog_dec(qdisc, skb);
- qdisc_bstats_cpu_update(qdisc, skb);
- qdisc_qstats_atomic_qlen_dec(qdisc);
+ qdisc_update_stats_at_dequeue(qdisc, skb);
} else {
qdisc->empty = true;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"
2019-04-08 16:35 [PATCH net-next 0/5] net: sched: move back qlen to per CPU accounting Paolo Abeni
` (3 preceding siblings ...)
2019-04-08 16:35 ` [PATCH net-next 4/5] net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too Paolo Abeni
@ 2019-04-08 16:35 ` Paolo Abeni
2019-04-08 21:17 ` Eric Dumazet
` (2 more replies)
4 siblings, 3 replies; 13+ messages in thread
From: Paolo Abeni @ 2019-04-08 16:35 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Eric Dumazet, Ivan Vecera
This revert commit 46b1c18f9deb ("net: sched: put back q.qlen
into a single location").
After the previous patch nobody accesses directly qlen for a child
qdisc when such qdisc does per CPU stats accounting.
In the control path nobody uses directly qlen since commit
677f1bc207c ("net: sched: introduce and use qdisc tree flush/purge
helpers"), so we can remove the contented atomic ops from the
datapath.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/sch_generic.h | 35 ++++++++++++++++++++---------------
net/core/gen_stats.c | 2 ++
net/sched/sch_generic.c | 9 +++++----
3 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index cdd1671bdd22..bee9d9eac9fa 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -52,10 +52,7 @@ struct qdisc_size_table {
struct qdisc_skb_head {
struct sk_buff *head;
struct sk_buff *tail;
- union {
- u32 qlen;
- atomic_t atomic_qlen;
- };
+ __u32 qlen;
spinlock_t lock;
};
@@ -474,19 +471,27 @@ static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
BUILD_BUG_ON(sizeof(qcb->data) < sz);
}
+static inline int qdisc_qlen_cpu(const struct Qdisc *q)
+{
+ return this_cpu_ptr(q->cpu_qstats)->qlen;
+}
+
static inline int qdisc_qlen(const struct Qdisc *q)
{
return q->q.qlen;
}
-static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
+static inline int qdisc_qlen_sum(const struct Qdisc *q)
{
- u32 qlen = q->qstats.qlen;
+ __u32 qlen = q->qstats.qlen;
+ int i;
- if (q->flags & TCQ_F_CPUSTATS)
- qlen += atomic_read(&q->q.atomic_qlen);
- else
+ if (q->flags & TCQ_F_CPUSTATS) {
+ for_each_possible_cpu(i)
+ qlen += per_cpu_ptr(q->cpu_qstats, i)->qlen;
+ } else {
qlen += q->q.qlen;
+ }
return qlen;
}
@@ -889,14 +894,14 @@ static inline void qdisc_qstats_cpu_backlog_inc(struct Qdisc *sch,
this_cpu_add(sch->cpu_qstats->backlog, qdisc_pkt_len(skb));
}
-static inline void qdisc_qstats_atomic_qlen_inc(struct Qdisc *sch)
+static inline void qdisc_qstats_cpu_qlen_inc(struct Qdisc *sch)
{
- atomic_inc(&sch->q.atomic_qlen);
+ this_cpu_inc(sch->cpu_qstats->qlen);
}
-static inline void qdisc_qstats_atomic_qlen_dec(struct Qdisc *sch)
+static inline void qdisc_qstats_cpu_qlen_dec(struct Qdisc *sch)
{
- atomic_dec(&sch->q.atomic_qlen);
+ this_cpu_dec(sch->cpu_qstats->qlen);
}
static inline void qdisc_qstats_cpu_requeues_inc(struct Qdisc *sch)
@@ -1112,7 +1117,7 @@ static inline void qdisc_update_stats_at_dequeue(struct Qdisc *sch,
if (qdisc_is_percpu_stats(sch)) {
qdisc_qstats_cpu_backlog_dec(sch, skb);
qdisc_bstats_cpu_update(sch, skb);
- qdisc_qstats_atomic_qlen_dec(sch);
+ qdisc_qstats_cpu_qlen_dec(sch);
} else {
qdisc_qstats_backlog_dec(sch, skb);
qdisc_bstats_update(sch, skb);
@@ -1124,7 +1129,7 @@ static inline void qdisc_update_stats_at_enqueue(struct Qdisc *sch,
unsigned pkt_len)
{
if (qdisc_is_percpu_stats(sch)) {
- qdisc_qstats_atomic_qlen_inc(sch);
+ qdisc_qstats_cpu_qlen_inc(sch);
this_cpu_add(sch->cpu_qstats->backlog, pkt_len);
} else {
sch->qstats.backlog += pkt_len;
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index ac679f74ba47..9bf1b9ad1780 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -291,6 +291,7 @@ __gnet_stats_copy_queue_cpu(struct gnet_stats_queue *qstats,
for_each_possible_cpu(i) {
const struct gnet_stats_queue *qcpu = per_cpu_ptr(q, i);
+ qstats->qlen = 0;
qstats->backlog += qcpu->backlog;
qstats->drops += qcpu->drops;
qstats->requeues += qcpu->requeues;
@@ -306,6 +307,7 @@ void __gnet_stats_copy_queue(struct gnet_stats_queue *qstats,
if (cpu) {
__gnet_stats_copy_queue_cpu(qstats, cpu);
} else {
+ qstats->qlen = q->qlen;
qstats->backlog = q->backlog;
qstats->drops = q->drops;
qstats->requeues = q->requeues;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 12a6e1a39fa0..848aab3693bd 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -68,7 +68,7 @@ static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
skb = __skb_dequeue(&q->skb_bad_txq);
if (qdisc_is_percpu_stats(q)) {
qdisc_qstats_cpu_backlog_dec(q, skb);
- qdisc_qstats_atomic_qlen_dec(q);
+ qdisc_qstats_cpu_qlen_dec(q);
} else {
qdisc_qstats_backlog_dec(q, skb);
q->q.qlen--;
@@ -108,7 +108,7 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
if (qdisc_is_percpu_stats(q)) {
qdisc_qstats_cpu_backlog_inc(q, skb);
- qdisc_qstats_atomic_qlen_inc(q);
+ qdisc_qstats_cpu_qlen_inc(q);
} else {
qdisc_qstats_backlog_inc(q, skb);
q->q.qlen++;
@@ -136,7 +136,7 @@ static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
if (qdisc_is_percpu_stats(q)) {
qdisc_qstats_cpu_requeues_inc(q);
qdisc_qstats_cpu_backlog_inc(q, skb);
- qdisc_qstats_atomic_qlen_inc(q);
+ qdisc_qstats_cpu_qlen_inc(q);
} else {
q->qstats.requeues++;
qdisc_qstats_backlog_inc(q, skb);
@@ -236,7 +236,7 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
skb = __skb_dequeue(&q->gso_skb);
if (qdisc_is_percpu_stats(q)) {
qdisc_qstats_cpu_backlog_dec(q, skb);
- qdisc_qstats_atomic_qlen_dec(q);
+ qdisc_qstats_cpu_qlen_dec(q);
} else {
qdisc_qstats_backlog_dec(q, skb);
q->q.qlen--;
@@ -694,6 +694,7 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
struct gnet_stats_queue *q = per_cpu_ptr(qdisc->cpu_qstats, i);
q->backlog = 0;
+ q->qlen = 0;
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"
2019-04-08 16:35 ` [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location" Paolo Abeni
@ 2019-04-08 21:17 ` Eric Dumazet
2019-04-09 7:52 ` Paolo Abeni
2019-04-09 9:51 ` kbuild test robot
2019-04-09 11:55 ` kbuild test robot
2 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2019-04-08 21:17 UTC (permalink / raw)
To: Paolo Abeni, netdev
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Eric Dumazet, Ivan Vecera
On 04/08/2019 09:35 AM, Paolo Abeni wrote:
> This revert commit 46b1c18f9deb ("net: sched: put back q.qlen
> into a single location").
> After the previous patch nobody accesses directly qlen for a child
> qdisc when such qdisc does per CPU stats accounting.
> In the control path nobody uses directly qlen since commit
> 677f1bc207c ("net: sched: introduce and use qdisc tree flush/purge
> helpers"), so we can remove the contented atomic ops from the
> datapath.
>
Have you tested HTB with a pfifo_fast on a throttled class ?
I do not see any changes in HTB in your patch series, so it is not
clear why your patch series is not adding back the issue.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"
2019-04-08 21:17 ` Eric Dumazet
@ 2019-04-09 7:52 ` Paolo Abeni
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Abeni @ 2019-04-09 7:52 UTC (permalink / raw)
To: Eric Dumazet, netdev
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Eric Dumazet, Ivan Vecera
Hi,
On Mon, 2019-04-08 at 14:17 -0700, Eric Dumazet wrote:
> On 04/08/2019 09:35 AM, Paolo Abeni wrote:
> > This revert commit 46b1c18f9deb ("net: sched: put back q.qlen
> > into a single location").
> > After the previous patch nobody accesses directly qlen for a child
> > qdisc when such qdisc does per CPU stats accounting.
> > In the control path nobody uses directly qlen since commit
> > 677f1bc207c ("net: sched: introduce and use qdisc tree flush/purge
> > helpers"), so we can remove the contented atomic ops from the
> > datapath.
> >
>
> Have you tested HTB with a pfifo_fast on a throttled class ?
>
> I do not see any changes in HTB in your patch series, so it is not
> clear why your patch series is not adding back the issue.
Thank you for the feedback.
I tested this series enslaving pfifo_fast to each classful qdiscs -
including HTB - sending traffic through the pfifo_fast qdisc, and
checking correct accounting.
When pfifo_fast is enslaved to HTB, the NOLOCK flag is cleared - by
qdisc_graft(), as HTB is a lock qdisc. As per patch 4/5, TCQ_F_CPUSTATS
is cleared, too, so pfifo_fast switches to global accounting, under
root lock protection.
In HTB context, <child pfifo_fast>->q.qlen should be always valid, no
changes required there, nor to any other classful qdisc - until we will
have lockless classful qdiscs: they should not access <child>->q.qlen
directly.
Please let me know if the above is somewhat clear. Have I missed
something?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"
2019-04-08 16:35 ` [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location" Paolo Abeni
2019-04-08 21:17 ` Eric Dumazet
@ 2019-04-09 9:51 ` kbuild test robot
2019-04-09 11:55 ` kbuild test robot
2 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-04-09 9:51 UTC (permalink / raw)
To: Paolo Abeni
Cc: kbuild-all, netdev, David S. Miller, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, Eric Dumazet, Ivan Vecera
[-- Attachment #1: Type: text/plain, Size: 5434 bytes --]
Hi Paolo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: i386-randconfig-x077-201914 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from include/net/cls_cgroup.h:19,
from net/socket.c:99:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
include/net/sch_generic.h:748:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
ida_is_empty
include/net/sch_generic.h: At top level:
include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^~~~~~~~~~~~~~
include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec'; did you mean 'qdisc_qstats_cpu_qlen_dec'? [-Werror=implicit-function-declaration]
qdisc_qstats_atomic_qlen_dec(sch);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
qdisc_qstats_cpu_qlen_dec
cc1: some warnings being treated as errors
--
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from include/net/inet_sock.h:26,
from include/net/inet_connection_sock.h:24,
from include/linux/dccp.h:13,
from net//dccp/proto.c:12:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
include/net/sch_generic.h:748:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
ida_is_empty
include/net/sch_generic.h: At top level:
include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^~~~~~~~~~~~~~
include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^~~~~~~~~~~~~~
include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec'; did you mean 'qdisc_qstats_cpu_qlen_dec'? [-Werror=implicit-function-declaration]
qdisc_qstats_atomic_qlen_dec(sch);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
qdisc_qstats_cpu_qlen_dec
In file included from net//dccp/trace.h:84:0,
from net//dccp/proto.c:42:
include/trace/define_trace.h: At top level:
include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
^
cc1: some warnings being treated as errors
compilation terminated.
vim +1149 include/net/sch_generic.h
fa70e3d2 Paolo Abeni 2019-04-08 1139
77be155c Jarek Poplawski 2008-10-31 1140 /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
77be155c Jarek Poplawski 2008-10-31 1141 static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
77be155c Jarek Poplawski 2008-10-31 1142 {
a53851e2 John Fastabend 2017-12-07 1143 struct sk_buff *skb = skb_peek(&sch->gso_skb);
77be155c Jarek Poplawski 2008-10-31 1144
61c9eaf9 Jarek Poplawski 2008-11-05 1145 if (skb) {
a53851e2 John Fastabend 2017-12-07 1146 skb = __skb_dequeue(&sch->gso_skb);
9cda4ff7 Paolo Abeni 2019-04-08 1147 if (qdisc_is_percpu_stats(sch)) {
9cda4ff7 Paolo Abeni 2019-04-08 1148 qdisc_qstats_cpu_backlog_dec(sch, skb);
9cda4ff7 Paolo Abeni 2019-04-08 @1149 qdisc_qstats_atomic_qlen_dec(sch);
9cda4ff7 Paolo Abeni 2019-04-08 1150 } else {
a27758ff WANG Cong 2016-06-03 1151 qdisc_qstats_backlog_dec(sch, skb);
61c9eaf9 Jarek Poplawski 2008-11-05 1152 sch->q.qlen--;
9cda4ff7 Paolo Abeni 2019-04-08 1153 }
61c9eaf9 Jarek Poplawski 2008-11-05 1154 } else {
77be155c Jarek Poplawski 2008-10-31 1155 skb = sch->dequeue(sch);
61c9eaf9 Jarek Poplawski 2008-11-05 1156 }
77be155c Jarek Poplawski 2008-10-31 1157
77be155c Jarek Poplawski 2008-10-31 1158 return skb;
77be155c Jarek Poplawski 2008-10-31 1159 }
77be155c Jarek Poplawski 2008-10-31 1160
:::::: The code at line 1149 was first introduced by commit
:::::: 9cda4ff7ed51bb469cb19e03c9fe4972408edb63 net: sched: always do stats accounting according to TCQ_F_CPUSTATS
:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32236 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"
2019-04-08 16:35 ` [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location" Paolo Abeni
2019-04-08 21:17 ` Eric Dumazet
2019-04-09 9:51 ` kbuild test robot
@ 2019-04-09 11:55 ` kbuild test robot
2 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-04-09 11:55 UTC (permalink / raw)
To: Paolo Abeni
Cc: kbuild-all, netdev, David S. Miller, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, Eric Dumazet, Ivan Vecera
[-- Attachment #1: Type: text/plain, Size: 4995 bytes --]
Hi Paolo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: i386-randconfig-b0-04091710 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from include/net/cls_cgroup.h:19,
from net/socket.c:99:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
include/net/sch_generic.h:748:3: error: implicit declaration of function 'qdisc_is_empty' [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^
include/net/sch_generic.h: At top level:
include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^
include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^
include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec' [-Werror=implicit-function-declaration]
qdisc_qstats_atomic_qlen_dec(sch);
^
cc1: some warnings being treated as errors
--
In file included from include/linux/filter.h:24:0,
from include/net/sock.h:64,
from net//tipc/socket.h:38,
from net//tipc/trace.h:45,
from net//tipc/trace.c:37:
include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
include/net/sch_generic.h:748:3: error: implicit declaration of function 'qdisc_is_empty' [-Werror=implicit-function-declaration]
if (!qdisc_is_empty(q)) {
^
include/net/sch_generic.h: At top level:
include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
^
include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
if (!qdisc_is_empty(q)) {
^
include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec' [-Werror=implicit-function-declaration]
qdisc_qstats_atomic_qlen_dec(sch);
^
In file included from net//tipc/trace.h:431:0,
from net//tipc/trace.c:37:
include/trace/define_trace.h: At top level:
include/trace/define_trace.h:89:43: fatal error: ./trace.h: No such file or directory
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
^
cc1: some warnings being treated as errors
compilation terminated.
vim +/qdisc_qstats_atomic_qlen_dec +1149 include/net/sch_generic.h
fa70e3d2 Paolo Abeni 2019-04-08 1139
77be155c Jarek Poplawski 2008-10-31 1140 /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
77be155c Jarek Poplawski 2008-10-31 1141 static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
77be155c Jarek Poplawski 2008-10-31 1142 {
a53851e2 John Fastabend 2017-12-07 1143 struct sk_buff *skb = skb_peek(&sch->gso_skb);
77be155c Jarek Poplawski 2008-10-31 1144
61c9eaf9 Jarek Poplawski 2008-11-05 1145 if (skb) {
a53851e2 John Fastabend 2017-12-07 1146 skb = __skb_dequeue(&sch->gso_skb);
9cda4ff7 Paolo Abeni 2019-04-08 1147 if (qdisc_is_percpu_stats(sch)) {
9cda4ff7 Paolo Abeni 2019-04-08 1148 qdisc_qstats_cpu_backlog_dec(sch, skb);
9cda4ff7 Paolo Abeni 2019-04-08 @1149 qdisc_qstats_atomic_qlen_dec(sch);
9cda4ff7 Paolo Abeni 2019-04-08 1150 } else {
a27758ff WANG Cong 2016-06-03 1151 qdisc_qstats_backlog_dec(sch, skb);
61c9eaf9 Jarek Poplawski 2008-11-05 1152 sch->q.qlen--;
9cda4ff7 Paolo Abeni 2019-04-08 1153 }
61c9eaf9 Jarek Poplawski 2008-11-05 1154 } else {
77be155c Jarek Poplawski 2008-10-31 1155 skb = sch->dequeue(sch);
61c9eaf9 Jarek Poplawski 2008-11-05 1156 }
77be155c Jarek Poplawski 2008-10-31 1157
77be155c Jarek Poplawski 2008-10-31 1158 return skb;
77be155c Jarek Poplawski 2008-10-31 1159 }
77be155c Jarek Poplawski 2008-10-31 1160
:::::: The code at line 1149 was first introduced by commit
:::::: 9cda4ff7ed51bb469cb19e03c9fe4972408edb63 net: sched: always do stats accounting according to TCQ_F_CPUSTATS
:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35508 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread