[PATCH] ppp: add 64 bit stats

[PATCH] ppp: add 64 bit stats

[Kevin Groeneveld]

Add 64 bit stats to ppp driver.  The 64 bit stats include tx_bytes,
rx_bytes, tx_packets and rx_packets.  Other stats are still 32 bit.
The 64 bit stats can be retrieved via the ndo_get_stats operation.  The
SIOCGPPPSTATS ioctl is still 32 bit stats only.

Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
---
 drivers/net/ppp/ppp_generic.c |  110 +++++++++++++++++++++++++++++++++++------
 1 file changed, 95 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 5c05572..210238c 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -94,6 +94,19 @@ struct ppp_file {
 #define PF_TO_CHANNEL(pf)	PF_TO_X(pf, struct channel)
 
 /*
+ * Data structure to hold primary network stats for which
+ * we want to use 64 bit storage.  Other network stats
+ * are stored in dev->stats of the ppp strucute.
+ */
+struct ppp_link_stats {
+	struct u64_stats_sync syncp;
+	u64 tx_bytes;
+	u64 tx_packets;
+	u64 rx_bytes;
+	u64 rx_packets;
+};
+
+/*
  * Data structure describing one ppp unit.
  * A ppp unit corresponds to a ppp network interface device
  * and represents a multilink bundle.
@@ -136,6 +149,7 @@ struct ppp {
 	unsigned pass_len, active_len;
 #endif /* CONFIG_PPP_FILTER */
 	struct net	*ppp_net;	/* the net we belong to */
+	struct ppp_link_stats __percpu *stats;	/* 64 bit network stats */
 };
 
 /*
@@ -1021,9 +1035,45 @@ ppp_net_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return err;
 }
 
+struct rtnl_link_stats64*
+ppp_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *tot)
+{
+	struct ppp *ppp = netdev_priv(dev);
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		struct ppp_link_stats *stats = per_cpu_ptr(ppp->stats, cpu);
+		unsigned int start;
+		u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
+
+		do {
+			start = u64_stats_fetch_begin_bh(&stats->syncp);
+			rx_packets = stats->rx_packets;
+			tx_packets = stats->tx_packets;
+			rx_bytes   = stats->rx_bytes;
+			tx_bytes   = stats->tx_bytes;
+
+		} while (u64_stats_fetch_retry_bh(&stats->syncp, start));
+
+		tot->rx_packets += rx_packets;
+		tot->tx_packets += tx_packets;
+		tot->rx_bytes   += rx_bytes;
+		tot->tx_bytes   += tx_bytes;
+	}
+
+	tot->rx_errors        = dev->stats.rx_errors;
+	tot->tx_errors        = dev->stats.tx_errors;
+	tot->rx_dropped       = dev->stats.rx_dropped;
+	tot->tx_dropped       = dev->stats.tx_dropped;
+	tot->rx_length_errors = dev->stats.rx_length_errors;
+
+	return tot;
+}
+
 static const struct net_device_ops ppp_netdev_ops = {
-	.ndo_start_xmit = ppp_start_xmit,
-	.ndo_do_ioctl   = ppp_net_ioctl,
+	.ndo_start_xmit  = ppp_start_xmit,
+	.ndo_do_ioctl    = ppp_net_ioctl,
+	.ndo_get_stats64 = ppp_get_stats64,
 };
 
 static void ppp_setup(struct net_device *dev)
@@ -1130,6 +1180,7 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
 	struct sk_buff *new_skb;
 	int len;
 	unsigned char *cp;
+	struct ppp_link_stats *stats = this_cpu_ptr(ppp->stats);
 
 	if (proto < 0x8000) {
 #ifdef CONFIG_PPP_FILTER
@@ -1157,8 +1208,10 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
 #endif /* CONFIG_PPP_FILTER */
 	}
 
-	++ppp->dev->stats.tx_packets;
-	ppp->dev->stats.tx_bytes += skb->len - 2;
+	u64_stats_update_begin(&stats->syncp);
+	++stats->tx_packets;
+	stats->tx_bytes += skb->len - 2;
+	u64_stats_update_end(&stats->syncp);
 
 	switch (proto) {
 	case PPP_IP:
@@ -1673,6 +1726,7 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
 {
 	struct sk_buff *ns;
 	int proto, len, npi;
+	struct ppp_link_stats *stats = this_cpu_ptr(ppp->stats);
 
 	/*
 	 * Decompress the frame, if compressed.
@@ -1745,8 +1799,10 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
 		break;
 	}
 
-	++ppp->dev->stats.rx_packets;
-	ppp->dev->stats.rx_bytes += skb->len - 2;
+	u64_stats_update_begin(&stats->syncp);
+	++stats->rx_packets;
+	stats->rx_bytes += skb->len - 2;
+	u64_stats_update_end(&stats->syncp);
 
 	npi = proto_to_npindex(proto);
 	if (npi < 0) {
@@ -2568,14 +2624,32 @@ static void
 ppp_get_stats(struct ppp *ppp, struct ppp_stats *st)
 {
 	struct slcompress *vj = ppp->vj;
+	int cpu;
 
 	memset(st, 0, sizeof(*st));
-	st->p.ppp_ipackets = ppp->dev->stats.rx_packets;
+
+	for_each_possible_cpu(cpu) {
+		struct ppp_link_stats *stats = per_cpu_ptr(ppp->stats, cpu);
+		unsigned int start;
+		u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
+
+		do {
+			start = u64_stats_fetch_begin_bh(&stats->syncp);
+			rx_packets = stats->rx_packets;
+			tx_packets = stats->tx_packets;
+			rx_bytes   = stats->rx_bytes;
+			tx_bytes   = stats->tx_bytes;
+
+		} while (u64_stats_fetch_retry_bh(&stats->syncp, start));
+
+		st->p.ppp_ipackets += rx_packets;
+		st->p.ppp_opackets += tx_packets;
+		st->p.ppp_ibytes   += rx_bytes;
+		st->p.ppp_obytes   += tx_bytes;
+	}
+
 	st->p.ppp_ierrors = ppp->dev->stats.rx_errors;
-	st->p.ppp_ibytes = ppp->dev->stats.rx_bytes;
-	st->p.ppp_opackets = ppp->dev->stats.tx_packets;
 	st->p.ppp_oerrors = ppp->dev->stats.tx_errors;
-	st->p.ppp_obytes = ppp->dev->stats.tx_bytes;
 	if (!vj)
 		return;
 	st->vj.vjs_packets = vj->sls_o_compressed + vj->sls_o_uncompressed;
@@ -2627,6 +2701,9 @@ ppp_create_interface(struct net *net, int unit, int *retp)
 	ppp->minseq = -1;
 	skb_queue_head_init(&ppp->mrq);
 #endif /* CONFIG_PPP_MULTILINK */
+	ppp->stats = alloc_percpu(struct ppp_link_stats);
+	if (ppp->stats == NULL)
+		goto out2;
 
 	/*
 	 * drum roll: don't forget to set
@@ -2640,12 +2717,12 @@ ppp_create_interface(struct net *net, int unit, int *retp)
 		unit = unit_get(&pn->units_idr, ppp);
 		if (unit < 0) {
 			ret = unit;
-			goto out2;
+			goto out3;
 		}
 	} else {
 		ret = -EEXIST;
 		if (unit_find(&pn->units_idr, unit))
-			goto out2; /* unit already exists */
+			goto out3; /* unit already exists */
 		/*
 		 * if caller need a specified unit number
 		 * lets try to satisfy him, otherwise --
@@ -2657,7 +2734,7 @@ ppp_create_interface(struct net *net, int unit, int *retp)
 		 */
 		unit = unit_set(&pn->units_idr, ppp, unit);
 		if (unit < 0)
-			goto out2;
+			goto out3;
 	}
 
 	/* Initialize the new ppp unit */
@@ -2669,7 +2746,7 @@ ppp_create_interface(struct net *net, int unit, int *retp)
 		unit_put(&pn->units_idr, unit);
 		netdev_err(ppp->dev, "PPP: couldn't register device %s (%d)\n",
 			   dev->name, ret);
-		goto out2;
+		goto out3;
 	}
 
 	ppp->ppp_net = net;
@@ -2680,8 +2757,10 @@ ppp_create_interface(struct net *net, int unit, int *retp)
 	*retp = 0;
 	return ppp;
 
-out2:
+out3:
 	mutex_unlock(&pn->all_ppp_mutex);
+	free_percpu(ppp->stats);
+out2:
 	free_netdev(dev);
 out1:
 	*retp = ret;
@@ -2765,6 +2844,7 @@ static void ppp_destroy_interface(struct ppp *ppp)
 
 	kfree_skb(ppp->xmit_pending);
 
+	free_percpu(ppp->stats);
 	free_netdev(ppp->dev);
 }
 
-- 
1.7.9.5

Re: [PATCH] ppp: add 64 bit stats

[David Miller]

From: Kevin Groeneveld <kgroeneveld@gmail.com>
Date: Sun, 22 Jul 2012 16:19:56 -0400

> Add 64 bit stats to ppp driver.  The 64 bit stats include tx_bytes,
> rx_bytes, tx_packets and rx_packets.  Other stats are still 32 bit.
> The 64 bit stats can be retrieved via the ndo_get_stats operation.  The
> SIOCGPPPSTATS ioctl is still 32 bit stats only.
> 
> Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>

I don't see this as being very practical nor justified.

Re: [PATCH] ppp: add 64 bit stats

[Kevin Groeneveld]

On Sun, Jul 22, 2012 at 5:54 PM, David Miller <davem@davemloft.net> wrote:
> From: Kevin Groeneveld <kgroeneveld@gmail.com>
> Date: Sun, 22 Jul 2012 16:19:56 -0400
>
>> Add 64 bit stats to ppp driver.  The 64 bit stats include tx_bytes,
>
> I don't see this as being very practical nor justified.

It is obviously not up to me to decide what is practical or justified
in this case.  I am just always annoyed when I log into my Linksys
router and check the network stats for my PPP interface and have no
idea how many times the transfer stats have rolled over at 4GB.


Kevin

Re: [PATCH] ppp: add 64 bit stats

[David Miller]

From: Kevin Groeneveld <kgroeneveld@gmail.com>
Date: Sun, 22 Jul 2012 20:32:26 -0400

> On Sun, Jul 22, 2012 at 5:54 PM, David Miller <davem@davemloft.net> wrote:
>> From: Kevin Groeneveld <kgroeneveld@gmail.com>
>> Date: Sun, 22 Jul 2012 16:19:56 -0400
>>
>>> Add 64 bit stats to ppp driver.  The 64 bit stats include tx_bytes,
>>
>> I don't see this as being very practical nor justified.
> 
> It is obviously not up to me to decide what is practical or justified
> in this case.  I am just always annoyed when I log into my Linksys
> router and check the network stats for my PPP interface and have no
> idea how many times the transfer stats have rolled over at 4GB.

I guess that's a good point.

Re: [PATCH] ppp: add 64 bit stats

[Eric Dumazet]

On Sun, 2012-07-22 at 16:19 -0400, Kevin Groeneveld wrote:
> Add 64 bit stats to ppp driver.  The 64 bit stats include tx_bytes,
> rx_bytes, tx_packets and rx_packets.  Other stats are still 32 bit.
> The 64 bit stats can be retrieved via the ndo_get_stats operation.  The
> SIOCGPPPSTATS ioctl is still 32 bit stats only.
> 
> Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
> ---
>  drivers/net/ppp/ppp_generic.c |  110 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 95 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 5c05572..210238c 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -94,6 +94,19 @@ struct ppp_file {
>  #define PF_TO_CHANNEL(pf)	PF_TO_X(pf, struct channel)
>  
>  /*
> + * Data structure to hold primary network stats for which
> + * we want to use 64 bit storage.  Other network stats
> + * are stored in dev->stats of the ppp strucute.
> + */
> +struct ppp_link_stats {
> +	struct u64_stats_sync syncp;
> +	u64 tx_bytes;
> +	u64 tx_packets;
> +	u64 rx_bytes;
> +	u64 rx_packets;
> +};

Hmm. This patches adds races, but also adds a good amount of memory on
these servers with thousand of ppp devices, and 64 cpus.

I really doubt ppp is performance sensitive, it so doesnt need percpu
counter.

If you really want 64bits stats on ppp, use proper synchronization
around u64 counters (but shared ones)

Re: [PATCH] ppp: add 64 bit stats

[Kevin Groeneveld]

Hi Eric,

On Mon, Jul 23, 2012 at 1:16 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Hmm. This patches adds races, but also adds a good amount of memory on
> these servers with thousand of ppp devices, and 64 cpus.

I am curious if you can elaborate on what is racy about the patch, I
am still trying to learn.  I assumed (possibly incorrectly) that
because I was using percpu variables that the stats updates didn't
need any extra synchronization as any concurrent updates would be on
different cpus.

> I really doubt ppp is performance sensitive, it so doesnt need percpu
> counter.
>
> If you really want 64bits stats on ppp, use proper synchronization
> around u64 counters (but shared ones)

I will work on an updated patch without the percpu variables.  I
didn't really think about servers with many cpus and many ppp sessions
when I created the patch, I was mainly thinking about my Linksys
router and other simple clients.  Many of the other network drivers
use percpu variables for their stats so I just followed along.

Would proper synchronization in this case just be wrapping the updates
in a spin_lock/spin_unlock?


Kevin

Re: [PATCH] ppp: add 64 bit stats

[Eric Dumazet]

On Mon, 2012-07-23 at 11:25 -0400, Kevin Groeneveld wrote:

> I am curious if you can elaborate on what is racy about the patch, I
> am still trying to learn.  I assumed (possibly incorrectly) that
> because I was using percpu variables that the stats updates didn't
> need any extra synchronization as any concurrent updates would be on
> different cpus.
> 

ppp paths (xmit versus receive) are reentrant.

Therefore several cpus might do the
u64_stats_update_begin(&stats->syncp) at the same moment : one increment
could be lost forever, making all readers looping forever in
u64_stats_fetch_begin_bh()

include/linux/u64_stats_sync.h

* 3) Write side must ensure mutual exclusion or one seqcount update could
 *    be lost, thus blocking readers forever.
 *    If this synchronization point is not a mutex, but a spinlock or
 *    spinlock_bh() or disable_bh() :


> > I really doubt ppp is performance sensitive, it so doesnt need percpu
> > counter.
> >
> > If you really want 64bits stats on ppp, use proper synchronization
> > around u64 counters (but shared ones)
> 
> I will work on an updated patch without the percpu variables.  I
> didn't really think about servers with many cpus and many ppp sessions
> when I created the patch, I was mainly thinking about my Linksys
> router and other simple clients.  Many of the other network drivers
> use percpu variables for their stats so I just followed along.

Because there is one loopback device only, not thousands ;)

> 
> Would proper synchronization in this case just be wrapping the updates
> in a spin_lock/spin_unlock?

Would be fine (if the proper BH safe variant is used), or you could also
use atomic64_t.

Re: [PATCH] ppp: add 64 bit stats

[Kevin Groeneveld]

On Mon, Jul 23, 2012 at 11:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Would proper synchronization in this case just be wrapping the updates
>> in a spin_lock/spin_unlock?
>
> Would be fine (if the proper BH safe variant is used), or you could also
> use atomic64_t.

Which would you recommend, spin locks or atomic64_t?

atomic64_t seems like it would be simpler.

Re: [PATCH] ppp: add 64 bit stats

[Eric Dumazet]

On Mon, 2012-07-23 at 21:53 -0400, Kevin Groeneveld wrote:
> On Mon, Jul 23, 2012 at 11:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> Would proper synchronization in this case just be wrapping the updates
> >> in a spin_lock/spin_unlock?
> >
> > Would be fine (if the proper BH safe variant is used), or you could also
> > use atomic64_t.
> 
> Which would you recommend, spin locks or atomic64_t?
> 
> atomic64_t seems like it would be simpler.

Simpler but a bit more expensive when two counters are changed at the
same time.

(two atomic ops instead of a single one for the spinlock)

Re: [PATCH] ppp: add 64 bit stats

[Kevin Groeneveld]

On Tue, Jul 24, 2012 at 2:22 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2012-07-23 at 21:53 -0400, Kevin Groeneveld wrote:
>> On Mon, Jul 23, 2012 at 11:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >> Would proper synchronization in this case just be wrapping the updates
>> >> in a spin_lock/spin_unlock?
>> >
>> > Would be fine (if the proper BH safe variant is used), or you could also
>> > use atomic64_t.
>>
>> Which would you recommend, spin locks or atomic64_t?
>>
>> atomic64_t seems like it would be simpler.
>
> Simpler but a bit more expensive when two counters are changed at the
> same time.
>
> (two atomic ops instead of a single one for the spinlock)

After looking at the existing ppp code more I wonder if I would even
need to add my own spinlocks.  It seems the only spots I am updating
the 64 bit stats are already protected by either ppp->wlock or
ppp->rlock spinlocks.  If I used two separate u64_stats_sync
structures, one for tx stats and one for rx stats, wouldn't the
existing spinlocks be enough?

Re: [PATCH] ppp: add 64 bit stats

[Eric Dumazet]

On Tue, 2012-07-24 at 22:27 -0400, Kevin Groeneveld wrote:

> After looking at the existing ppp code more I wonder if I would even
> need to add my own spinlocks.  It seems the only spots I am updating
> the 64 bit stats are already protected by either ppp->wlock or
> ppp->rlock spinlocks.  If I used two separate u64_stats_sync
> structures, one for tx stats and one for rx stats, wouldn't the
> existing spinlocks be enough?

It there are spinlocks already, why even adding u64_stats_sync ?

Just use existing spinlocks to protect your new u64 fields (in the
ngso_get_stats64())

Re: [PATCH] ppp: add 64 bit stats

[Kevin Groeneveld]

On Wed, Jul 25, 2012 at 12:04 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> It there are spinlocks already, why even adding u64_stats_sync ?

That is a good question I have already wondered about myself.

>include/linux/u64_stats_sync.h
>
>* 3) Write side must ensure mutual exclusion or one seqcount update could
> *    be lost, thus blocking readers forever.
> *    If this synchronization point is not a mutex, but a spinlock or
> *    spinlock_bh() or disable_bh() :

It seems the u64_stats_sync requires some form of mutual exclusion.
So why bother ever using it at all?  Maybe there are cases where the
required mutual exclusion can be cheaper than a spinlock?  Maybe it is
just to avoid the spinlocks on the read side of things?

I hope you don't mind all my questions...

Re: [PATCH] ppp: add 64 bit stats

[Eric Dumazet]

On Wed, 2012-07-25 at 10:43 -0400, Kevin Groeneveld wrote:
> On Wed, Jul 25, 2012 at 12:04 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > It there are spinlocks already, why even adding u64_stats_sync ?
> 
> That is a good question I have already wondered about myself.
> 
> >include/linux/u64_stats_sync.h
> >
> >* 3) Write side must ensure mutual exclusion or one seqcount update could
> > *    be lost, thus blocking readers forever.
> > *    If this synchronization point is not a mutex, but a spinlock or
> > *    spinlock_bh() or disable_bh() :
> 
> It seems the u64_stats_sync requires some form of mutual exclusion.
> So why bother ever using it at all?  Maybe there are cases where the
> required mutual exclusion can be cheaper than a spinlock?  Maybe it is
> just to avoid the spinlocks on the read side of things?
> 
> I hope you don't mind all my questions...

u64_stats_sync is good for write sides without any locking,
for example using percpu data.

So if you use shared counters, u64_stats_sync has no value.