Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats
@ 2019-11-28  6:30 Dust Li
  2019-11-30  5:44 ` Cong Wang
  2019-11-30 20:22 ` David Miller
  0 siblings, 2 replies; 5+ messages in thread
From: Dust Li @ 2019-11-28  6:30 UTC (permalink / raw)
  To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, John Fastabend; +Cc: Tony Lu, netdev

__gnet_stats_copy_basic/queue() support both percpu stat and
non-percpu stat, but they are handle in a different manner:
1. For percpu stat, percpu stats are added to the return value;
2. For non-percpu stat, non-percpu stats will overwrite the
   return value;
We should keep the same semantics for both type.

This patch makes percpu stats follow non-percpu's manner by
reset the return bstats before add the percpu bstats to it.
Also changes the caller in sch_mq.c/sch_mqprio.c to make sure
they dump the right statistics for percpu qdisc.

One more thing, the sch->q.qlen is not set with nonlock child
qdisc in mq_dump()/mqprio_dump(), add that.

Fixes: 22e0f8b9322c ("net: sched: make bstats per cpu and estimator RCU safe")
Signed-off-by: Dust Li <dust.li@linux.alibaba.com>
Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
---
 net/core/gen_stats.c   |  2 ++
 net/sched/sch_mq.c     | 34 ++++++++++++++++------------------
 net/sched/sch_mqprio.c | 35 +++++++++++++++++------------------
 3 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 1d653fbfcf52..d71af69196c9 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -120,6 +120,7 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
 {
 	int i;
 
+	memset(bstats, 0, sizeof(*bstats));
 	for_each_possible_cpu(i) {
 		struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
 		unsigned int start;
@@ -288,6 +289,7 @@ __gnet_stats_copy_queue_cpu(struct gnet_stats_queue *qstats,
 {
 	int i;
 
+	memset(qstats, 0, sizeof(*qstats));
 	for_each_possible_cpu(i) {
 		const struct gnet_stats_queue *qcpu = per_cpu_ptr(q, i);
 
diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index 278c0b2dc523..b2178b7fe3a3 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -131,6 +131,8 @@ static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct Qdisc *qdisc;
 	unsigned int ntx;
 	__u32 qlen = 0;
+	struct gnet_stats_queue qstats = {0};
+	struct gnet_stats_basic_packed bstats = {0};
 
 	sch->q.qlen = 0;
 	memset(&sch->bstats, 0, sizeof(sch->bstats));
@@ -145,24 +147,20 @@ static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
 		qdisc = netdev_get_tx_queue(dev, ntx)->qdisc_sleeping;
 		spin_lock_bh(qdisc_lock(qdisc));
 
-		if (qdisc_is_percpu_stats(qdisc)) {
-			qlen = qdisc_qlen_sum(qdisc);
-			__gnet_stats_copy_basic(NULL, &sch->bstats,
-						qdisc->cpu_bstats,
-						&qdisc->bstats);
-			__gnet_stats_copy_queue(&sch->qstats,
-						qdisc->cpu_qstats,
-						&qdisc->qstats, qlen);
-		} else {
-			sch->q.qlen		+= qdisc->q.qlen;
-			sch->bstats.bytes	+= qdisc->bstats.bytes;
-			sch->bstats.packets	+= qdisc->bstats.packets;
-			sch->qstats.qlen	+= qdisc->qstats.qlen;
-			sch->qstats.backlog	+= qdisc->qstats.backlog;
-			sch->qstats.drops	+= qdisc->qstats.drops;
-			sch->qstats.requeues	+= qdisc->qstats.requeues;
-			sch->qstats.overlimits	+= qdisc->qstats.overlimits;
-		}
+		qlen = qdisc_qlen_sum(qdisc);
+		__gnet_stats_copy_basic(NULL, &bstats, qdisc->cpu_bstats,
+					&qdisc->bstats);
+		__gnet_stats_copy_queue(&qstats, qdisc->cpu_qstats,
+					&qdisc->qstats, qlen);
+
+		sch->q.qlen		+= qdisc->q.qlen;
+		sch->bstats.bytes	+= bstats.bytes;
+		sch->bstats.packets	+= bstats.packets;
+		sch->qstats.qlen	+= qstats.qlen;
+		sch->qstats.backlog	+= qstats.backlog;
+		sch->qstats.drops	+= qstats.drops;
+		sch->qstats.requeues	+= qstats.requeues;
+		sch->qstats.overlimits	+= qstats.overlimits;
 
 		spin_unlock_bh(qdisc_lock(qdisc));
 	}
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 0d0113a24962..6887084bd5ad 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -388,6 +388,9 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct tc_mqprio_qopt opt = { 0 };
 	struct Qdisc *qdisc;
 	unsigned int ntx, tc;
+	__u32 qlen = 0;
+	struct gnet_stats_queue qstats = {0};
+	struct gnet_stats_basic_packed bstats = {0};
 
 	sch->q.qlen = 0;
 	memset(&sch->bstats, 0, sizeof(sch->bstats));
@@ -402,24 +405,20 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 		qdisc = netdev_get_tx_queue(dev, ntx)->qdisc_sleeping;
 		spin_lock_bh(qdisc_lock(qdisc));
 
-		if (qdisc_is_percpu_stats(qdisc)) {
-			__u32 qlen = qdisc_qlen_sum(qdisc);
-
-			__gnet_stats_copy_basic(NULL, &sch->bstats,
-						qdisc->cpu_bstats,
-						&qdisc->bstats);
-			__gnet_stats_copy_queue(&sch->qstats,
-						qdisc->cpu_qstats,
-						&qdisc->qstats, qlen);
-		} else {
-			sch->q.qlen		+= qdisc->q.qlen;
-			sch->bstats.bytes	+= qdisc->bstats.bytes;
-			sch->bstats.packets	+= qdisc->bstats.packets;
-			sch->qstats.backlog	+= qdisc->qstats.backlog;
-			sch->qstats.drops	+= qdisc->qstats.drops;
-			sch->qstats.requeues	+= qdisc->qstats.requeues;
-			sch->qstats.overlimits	+= qdisc->qstats.overlimits;
-		}
+		qlen = qdisc_qlen_sum(qdisc);
+		__gnet_stats_copy_basic(NULL, &bstats, qdisc->cpu_bstats,
+					&qdisc->bstats);
+		__gnet_stats_copy_queue(&qstats, qdisc->cpu_qstats,
+					&qdisc->qstats, qlen);
+
+		sch->q.qlen		+= qdisc->q.qlen;
+		sch->bstats.bytes	+= bstats.bytes;
+		sch->bstats.packets	+= bstats.packets;
+		sch->qstats.qlen	+= qstats.qlen;
+		sch->qstats.backlog	+= qstats.backlog;
+		sch->qstats.drops	+= qstats.drops;
+		sch->qstats.requeues	+= qstats.requeues;
+		sch->qstats.overlimits	+= qstats.overlimits;
 
 		spin_unlock_bh(qdisc_lock(qdisc));
 	}
-- 
2.19.1.3.ge56e4f7


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats
  2019-11-28  6:30 [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats Dust Li
@ 2019-11-30  5:44 ` Cong Wang
  2019-12-02 11:15   ` Dust Li
  2019-11-30 20:22 ` David Miller
  1 sibling, 1 reply; 5+ messages in thread
From: Cong Wang @ 2019-11-30  5:44 UTC (permalink / raw)
  To: Dust Li
  Cc: Jamal Hadi Salim, Jiri Pirko, John Fastabend, Tony Lu,
	Linux Kernel Network Developers

On Wed, Nov 27, 2019 at 10:31 PM Dust Li <dust.li@linux.alibaba.com> wrote:
>
> __gnet_stats_copy_basic/queue() support both percpu stat and
> non-percpu stat, but they are handle in a different manner:
> 1. For percpu stat, percpu stats are added to the return value;
> 2. For non-percpu stat, non-percpu stats will overwrite the
>    return value;
> We should keep the same semantics for both type.
>
> This patch makes percpu stats follow non-percpu's manner by
> reset the return bstats before add the percpu bstats to it.
> Also changes the caller in sch_mq.c/sch_mqprio.c to make sure
> they dump the right statistics for percpu qdisc.
>
> One more thing, the sch->q.qlen is not set with nonlock child
> qdisc in mq_dump()/mqprio_dump(), add that.
>
> Fixes: 22e0f8b9322c ("net: sched: make bstats per cpu and estimator RCU safe")
> Signed-off-by: Dust Li <dust.li@linux.alibaba.com>
> Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
> ---
>  net/core/gen_stats.c   |  2 ++
>  net/sched/sch_mq.c     | 34 ++++++++++++++++------------------
>  net/sched/sch_mqprio.c | 35 +++++++++++++++++------------------
>  3 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 1d653fbfcf52..d71af69196c9 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -120,6 +120,7 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
>  {
>         int i;
>
> +       memset(bstats, 0, sizeof(*bstats));
>         for_each_possible_cpu(i) {
>                 struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
>                 unsigned int start;
> @@ -288,6 +289,7 @@ __gnet_stats_copy_queue_cpu(struct gnet_stats_queue *qstats,
>  {
>         int i;
>
> +       memset(qstats, 0, sizeof(*qstats));


I think its caller is responsible to clear the stats, so you don't need to
clear them here? It looks like you do memset() twice.

Does this patch fix any bug? It looks more like a clean up to me, if so
please mark it for net-next.

Thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats
  2019-11-28  6:30 [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats Dust Li
  2019-11-30  5:44 ` Cong Wang
@ 2019-11-30 20:22 ` David Miller
  2019-12-02 11:17   ` Dust Li
  1 sibling, 1 reply; 5+ messages in thread
From: David Miller @ 2019-11-30 20:22 UTC (permalink / raw)
  To: dust.li; +Cc: jhs, xiyou.wangcong, jiri, john.fastabend, tonylu, netdev

From: Dust Li <dust.li@linux.alibaba.com>
Date: Thu, 28 Nov 2019 14:30:48 +0800

> __gnet_stats_copy_basic/queue() support both percpu stat and
> non-percpu stat, but they are handle in a different manner:
> 1. For percpu stat, percpu stats are added to the return value;
> 2. For non-percpu stat, non-percpu stats will overwrite the
>    return value;
> We should keep the same semantics for both type.
> 
> This patch makes percpu stats follow non-percpu's manner by
> reset the return bstats before add the percpu bstats to it.
> Also changes the caller in sch_mq.c/sch_mqprio.c to make sure
> they dump the right statistics for percpu qdisc.
> 
> One more thing, the sch->q.qlen is not set with nonlock child
> qdisc in mq_dump()/mqprio_dump(), add that.
> 
> Fixes: 22e0f8b9322c ("net: sched: make bstats per cpu and estimator RCU safe")
> Signed-off-by: Dust Li <dust.li@linux.alibaba.com>
> Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>

You are changing way too many things at one time here.

Fix one bug in one patch, for example just fix the missed
initialization of the per-cpu stats.

The qlen fix is another patch.

And so on and so forth.

Thank you.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats
  2019-11-30  5:44 ` Cong Wang
@ 2019-12-02 11:15   ` Dust Li
  0 siblings, 0 replies; 5+ messages in thread
From: Dust Li @ 2019-12-02 11:15 UTC (permalink / raw)
  To: Cong Wang
  Cc: Jamal Hadi Salim, Jiri Pirko, John Fastabend, Tony Lu,
	Linux Kernel Network Developers


On 11/30/19 1:44 PM, Cong Wang wrote:
> On Wed, Nov 27, 2019 at 10:31 PM Dust Li <dust.li@linux.alibaba.com> wrote:
>> __gnet_stats_copy_basic/queue() support both percpu stat and
>> non-percpu stat, but they are handle in a different manner:
>> 1. For percpu stat, percpu stats are added to the return value;
>> 2. For non-percpu stat, non-percpu stats will overwrite the
>>     return value;
>> We should keep the same semantics for both type.
>>
>> This patch makes percpu stats follow non-percpu's manner by
>> reset the return bstats before add the percpu bstats to it.
>> Also changes the caller in sch_mq.c/sch_mqprio.c to make sure
>> they dump the right statistics for percpu qdisc.
>>
>> One more thing, the sch->q.qlen is not set with nonlock child
>> qdisc in mq_dump()/mqprio_dump(), add that.
>>
>> Fixes: 22e0f8b9322c ("net: sched: make bstats per cpu and estimator RCU safe")
>> Signed-off-by: Dust Li <dust.li@linux.alibaba.com>
>> Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
>> ---
>>   net/core/gen_stats.c   |  2 ++
>>   net/sched/sch_mq.c     | 34 ++++++++++++++++------------------
>>   net/sched/sch_mqprio.c | 35 +++++++++++++++++------------------
>>   3 files changed, 35 insertions(+), 36 deletions(-)
>>
>> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
>> index 1d653fbfcf52..d71af69196c9 100644
>> --- a/net/core/gen_stats.c
>> +++ b/net/core/gen_stats.c
>> @@ -120,6 +120,7 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
>>   {
>>          int i;
>>
>> +       memset(bstats, 0, sizeof(*bstats));
>>          for_each_possible_cpu(i) {
>>                  struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
>>                  unsigned int start;
>> @@ -288,6 +289,7 @@ __gnet_stats_copy_queue_cpu(struct gnet_stats_queue *qstats,
>>   {
>>          int i;
>>
>> +       memset(qstats, 0, sizeof(*qstats));
>
> I think its caller is responsible to clear the stats, so you don't need to
> clear them here? It looks like you do memset() twice.

Yes, I should do this in its caller. I will change it, thanks.

The memset() is in two different functions, one for xxx_basic_cpu(), and the

other for xxx_queue_cpu().

> Does this patch fix any bug? It looks more like a clean up to me, if so
> please mark it for net-next.

It only fixes the 'sch->q.qlen' not set for NOLOCK child qdisc. But as 
Dave said,

I should split that into an individual patch. So I will change this and 
mark it

for net-next.


Thanks

Dust Li


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats
  2019-11-30 20:22 ` David Miller
@ 2019-12-02 11:17   ` Dust Li
  0 siblings, 0 replies; 5+ messages in thread
From: Dust Li @ 2019-12-02 11:17 UTC (permalink / raw)
  To: David Miller; +Cc: jhs, xiyou.wangcong, jiri, john.fastabend, tonylu, netdev


On 12/1/19 4:22 AM, David Miller wrote:
> From: Dust Li <dust.li@linux.alibaba.com>
> Date: Thu, 28 Nov 2019 14:30:48 +0800
>
>> __gnet_stats_copy_basic/queue() support both percpu stat and
>> non-percpu stat, but they are handle in a different manner:
>> 1. For percpu stat, percpu stats are added to the return value;
>> 2. For non-percpu stat, non-percpu stats will overwrite the
>>     return value;
>> We should keep the same semantics for both type.
>>
>> This patch makes percpu stats follow non-percpu's manner by
>> reset the return bstats before add the percpu bstats to it.
>> Also changes the caller in sch_mq.c/sch_mqprio.c to make sure
>> they dump the right statistics for percpu qdisc.
>>
>> One more thing, the sch->q.qlen is not set with nonlock child
>> qdisc in mq_dump()/mqprio_dump(), add that.
>>
>> Fixes: 22e0f8b9322c ("net: sched: make bstats per cpu and estimator RCU safe")
>> Signed-off-by: Dust Li <dust.li@linux.alibaba.com>
>> Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
> You are changing way too many things at one time here.
>
> Fix one bug in one patch, for example just fix the missed
> initialization of the per-cpu stats.
>
> The qlen fix is another patch.
>
> And so on and so forth.
>
> Thank you.

OK, I will separate them. Thanks for review !


Thanks.

Dust Li


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28  6:30 [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats Dust Li
2019-11-30  5:44 ` Cong Wang
2019-12-02 11:15   ` Dust Li
2019-11-30 20:22 ` David Miller
2019-12-02 11:17   ` Dust Li

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git