[PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[fgao]

From: Gao Feng <fgao@ikuai8.com>

The dropped count of veth is located in struct veth_priv, but other
statistics like packets and bytes are in another struct pcpu_vstats.
Now keep these three counters in the same struct.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 v2: Use right "peer" instead of "dev";
 v1: Initial version

 drivers/net/veth.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 0520952a..0d669b4 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -26,12 +26,12 @@
 struct pcpu_vstats {
 	u64			packets;
 	u64			bytes;
+	u64			dropped;
 	struct u64_stats_sync	syncp;
 };
 
 struct veth_priv {
 	struct net_device __rcu	*peer;
-	atomic64_t		dropped;
 	unsigned		requested_headroom;
 };
 
@@ -108,6 +108,8 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct veth_priv *priv = netdev_priv(dev);
 	struct net_device *rcv;
 	int length = skb->len;
+	struct pcpu_vstats *stats = this_cpu_ptr(dev->vstats);
+	int ret = NET_RX_DROP;
 
 	rcu_read_lock();
 	rcv = rcu_dereference(priv->peer);
@@ -116,17 +118,16 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto drop;
 	}
 
-	if (likely(dev_forward_skb(rcv, skb) == NET_RX_SUCCESS)) {
-		struct pcpu_vstats *stats = this_cpu_ptr(dev->vstats);
-
-		u64_stats_update_begin(&stats->syncp);
+	ret = dev_forward_skb(rcv, skb);
+drop:
+	u64_stats_update_begin(&stats->syncp);
+	if (likely(ret == NET_RX_SUCCESS)) {
 		stats->bytes += length;
 		stats->packets++;
-		u64_stats_update_end(&stats->syncp);
 	} else {
-drop:
-		atomic64_inc(&priv->dropped);
+		stats->dropped++;
 	}
+	u64_stats_update_end(&stats->syncp);
 	rcu_read_unlock();
 	return NETDEV_TX_OK;
 }
@@ -135,27 +136,28 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
  * general routines
  */
 
-static u64 veth_stats_one(struct pcpu_vstats *result, struct net_device *dev)
+static void veth_stats_one(struct pcpu_vstats *result, struct net_device *dev)
 {
-	struct veth_priv *priv = netdev_priv(dev);
 	int cpu;
 
 	result->packets = 0;
 	result->bytes = 0;
+	result->dropped = 0;
 	for_each_possible_cpu(cpu) {
 		struct pcpu_vstats *stats = per_cpu_ptr(dev->vstats, cpu);
-		u64 packets, bytes;
+		u64 packets, bytes, dropped;
 		unsigned int start;
 
 		do {
 			start = u64_stats_fetch_begin_irq(&stats->syncp);
 			packets = stats->packets;
 			bytes = stats->bytes;
+			dropped = stats->dropped;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
 		result->packets += packets;
 		result->bytes += bytes;
+		result->dropped += dropped;
 	}
-	return atomic64_read(&priv->dropped);
 }
 
 static struct rtnl_link_stats64 *veth_get_stats64(struct net_device *dev,
@@ -165,16 +167,18 @@ static struct rtnl_link_stats64 *veth_get_stats64(struct net_device *dev,
 	struct net_device *peer;
 	struct pcpu_vstats one;
 
-	tot->tx_dropped = veth_stats_one(&one, dev);
+	veth_stats_one(&one, dev);
 	tot->tx_bytes = one.bytes;
 	tot->tx_packets = one.packets;
+	tot->tx_dropped = one.dropped;
 
 	rcu_read_lock();
 	peer = rcu_dereference(priv->peer);
 	if (peer) {
-		tot->rx_dropped = veth_stats_one(&one, peer);
+		veth_stats_one(&one, peer);
 		tot->rx_bytes = one.bytes;
 		tot->rx_packets = one.packets;
+		tot->rx_dropped = one.dropped;
 	}
 	rcu_read_unlock();
 
-- 
1.9.1

Re: [PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[Eric Dumazet]

On Thu, 2016-11-03 at 21:03 +0800, fgao@ikuai8.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
> 
> The dropped count of veth is located in struct veth_priv, but other
> statistics like packets and bytes are in another struct pcpu_vstats.
> Now keep these three counters in the same struct.
> 
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>  v2: Use right "peer" instead of "dev";
>  v1: Initial version

May I ask : Why ?

We did that because there was no point making per-cpu requirements
bigger, for a counter that is hardly ever updated.

Do you have a real case where performance dropping packets in a driver
is needed ?

At some point we will have to stop dumb percpu explosion, when we have
128+ cores per host. Folding all these percpu counters is taking a lot
of time too.

Re: [PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[Gao Feng]

Hi Eric,

On Thu, Nov 3, 2016 at 9:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-11-03 at 21:03 +0800, fgao@ikuai8.com wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> The dropped count of veth is located in struct veth_priv, but other
>> statistics like packets and bytes are in another struct pcpu_vstats.
>> Now keep these three counters in the same struct.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>>  v2: Use right "peer" instead of "dev";
>>  v1: Initial version
>
> May I ask : Why ?

Just because I think statistics should be in the same struct.

>
> We did that because there was no point making per-cpu requirements
> bigger, for a counter that is hardly ever updated.
>
> Do you have a real case where performance dropping packets in a driver
> is needed ?

No, I haven't met the performance issue now.

>
> At some point we will have to stop dumb percpu explosion, when we have
> 128+ cores per host. Folding all these percpu counters is taking a lot
> of time too.
>
>
>
Ok, I get it. It is designed specially to put the dropped counter as
atomic counter, not percpu.
But I have one question that when put the counters as percpu, and when not?

Regards
Feng

Re: [PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[Eric Dumazet]

On Thu, 2016-11-03 at 21:39 +0800, Gao Feng wrote:
> Hi Eric,
> 
> On Thu, Nov 3, 2016 at 9:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2016-11-03 at 21:03 +0800, fgao@ikuai8.com wrote:
> >> From: Gao Feng <fgao@ikuai8.com>
> >>
> >> The dropped count of veth is located in struct veth_priv, but other
> >> statistics like packets and bytes are in another struct pcpu_vstats.
> >> Now keep these three counters in the same struct.
> >>
> >> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> >> ---
> >>  v2: Use right "peer" instead of "dev";
> >>  v1: Initial version
> >
> > May I ask : Why ?
> 
> Just because I think statistics should be in the same struct.

That is not a good reason then.

> 
> >
> > We did that because there was no point making per-cpu requirements
> > bigger, for a counter that is hardly ever updated.
> >
> > Do you have a real case where performance dropping packets in a driver
> > is needed ?
> 
> No, I haven't met the performance issue now.

OK then kill this patch.

> 
> >
> > At some point we will have to stop dumb percpu explosion, when we have
> > 128+ cores per host. Folding all these percpu counters is taking a lot
> > of time too.
> >
> >
> >
> Ok, I get it. It is designed specially to put the dropped counter as
> atomic counter, not percpu.
> But I have one question that when put the counters as percpu, and when not?

Because the regular fast path needs to be fast ?

Try to _use_ veth without these percpu stats and be prepared to be
shocked.

Re: [PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[Gao Feng]

On Thu, Nov 3, 2016 at 10:31 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-11-03 at 21:39 +0800, Gao Feng wrote:
>> Hi Eric,
>>
>> On Thu, Nov 3, 2016 at 9:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Thu, 2016-11-03 at 21:03 +0800, fgao@ikuai8.com wrote:
>> >> From: Gao Feng <fgao@ikuai8.com>
>> >>
>> >> The dropped count of veth is located in struct veth_priv, but other
>> >> statistics like packets and bytes are in another struct pcpu_vstats.
>> >> Now keep these three counters in the same struct.
>> >>
>> >> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> >> ---
>> >>  v2: Use right "peer" instead of "dev";
>> >>  v1: Initial version
>> >
>> > May I ask : Why ?
>>
>> Just because I think statistics should be in the same struct.
>
> That is not a good reason then.

Because other net devices put the statistics together.
Take tun/tap as example, it is a virtual device, but its all
statistics are percpu including dropped.

Regards
Feng

>
>>
>> >
>> > We did that because there was no point making per-cpu requirements
>> > bigger, for a counter that is hardly ever updated.
>> >
>> > Do you have a real case where performance dropping packets in a driver
>> > is needed ?
>>
>> No, I haven't met the performance issue now.
>
> OK then kill this patch.
>
>>
>> >
>> > At some point we will have to stop dumb percpu explosion, when we have
>> > 128+ cores per host. Folding all these percpu counters is taking a lot
>> > of time too.
>> >
>> >
>> >
>> Ok, I get it. It is designed specially to put the dropped counter as
>> atomic counter, not percpu.
>> But I have one question that when put the counters as percpu, and when not?
>
> Because the regular fast path needs to be fast ?
>
> Try to _use_ veth without these percpu stats and be prepared to be
> shocked.
>
>
>

Re: [PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[Eric Dumazet]

On Thu, 2016-11-03 at 22:38 +0800, Gao Feng wrote:

> Because other net devices put the statistics together.
> Take tun/tap as example, it is a virtual device, but its all
> statistics are percpu including dropped.

Take a look at 2681128f0ced8aa4e66f221197e183cc16d244fe
("veth: reduce stat overhead")

Feel free to fix tun/tap, not bloat veth and undo my work,
without knowing why it was done this way.

Thanks.

Re: [PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[Gao Feng]

Hi Eric,

On Thu, Nov 3, 2016 at 11:07 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-11-03 at 22:38 +0800, Gao Feng wrote:
>
>> Because other net devices put the statistics together.
>> Take tun/tap as example, it is a virtual device, but its all
>> statistics are percpu including dropped.
>
> Take a look at 2681128f0ced8aa4e66f221197e183cc16d244fe
> ("veth: reduce stat overhead")
>
> Feel free to fix tun/tap, not bloat veth and undo my work,
> without knowing why it was done this way.
>
> Thanks.
>
>

Thanks your detail explanations.

Best Regards
Feng

Re: [PATCH net-next v2 1/1] driver: veth: Refine the statistics codes of veth driver

[David Miller]

From: Gao Feng <fgao@ikuai8.com>
Date: Thu, 3 Nov 2016 22:38:28 +0800

> On Thu, Nov 3, 2016 at 10:31 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Thu, 2016-11-03 at 21:39 +0800, Gao Feng wrote:
>>> Hi Eric,
>>>
>>> On Thu, Nov 3, 2016 at 9:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> > On Thu, 2016-11-03 at 21:03 +0800, fgao@ikuai8.com wrote:
>>> >> From: Gao Feng <fgao@ikuai8.com>
>>> >>
>>> >> The dropped count of veth is located in struct veth_priv, but other
>>> >> statistics like packets and bytes are in another struct pcpu_vstats.
>>> >> Now keep these three counters in the same struct.
>>> >>
>>> >> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>>> >> ---
>>> >>  v2: Use right "peer" instead of "dev";
>>> >>  v1: Initial version
>>> >
>>> > May I ask : Why ?
>>>
>>> Just because I think statistics should be in the same struct.
>>
>> That is not a good reason then.
> 
> Because other net devices put the statistics together.

Organizational "prettyness" is not argument for this change, when the
downsides are fundamentally clear:

1) It is not a fast-path accessed statistic, so the per-cpu'ness is
   not important.

2) We aim to minimize the amount of per-cpu data in the kernel because
   it is expensive.  So when not necessary, as is the case here, we
   do not user per-cpu data.

There are no good reasons to make this change, so I am dropping your
patch.