[PATCH] macvlan: lockless tx path

[PATCH] macvlan: lockless tx path

[Eric Dumazet]

macvlan is a stacked device, like tunnels. We should use the lockless
mechanism we are using in tunnels and loopback.

This patch completely removes locking in TX path.

tx stat counters are added into existing percpu stat structure, renamed
from rx_stats to pcpu_stats.

Note : this reverts commit 2c11455321f37 (macvlan: add multiqueue
capability)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/macvlan.c      |   72 +++++++++++++----------------------
 include/linux/if_macvlan.h |   26 +++++++-----
 2 files changed, 43 insertions(+), 55 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 0fc9dc7..fce80a7 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -243,17 +243,19 @@ xmit_world:
 netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
 			       struct net_device *dev)
 {
-	int i = skb_get_queue_mapping(skb);
-	struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
 	unsigned int len = skb->len;
 	int ret;
+	const struct macvlan_dev *vlan = netdev_priv(dev);
+	struct macvlan_pcpu_stats *pcpu_stats = this_cpu_ptr(vlan->pcpu_stats);
 
 	ret = macvlan_queue_xmit(skb, dev);
 	if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) {
-		txq->tx_packets++;
-		txq->tx_bytes += len;
+		u64_stats_update_begin(&pcpu_stats->syncp);
+		pcpu_stats->tx_packets++;
+		pcpu_stats->tx_bytes += len;
+		u64_stats_update_end(&pcpu_stats->syncp);
 	} else
-		txq->tx_dropped++;
+		pcpu_stats->tx_dropped++;
 
 	return ret;
 }
@@ -414,14 +416,15 @@ static int macvlan_init(struct net_device *dev)
 	dev->state		= (dev->state & ~MACVLAN_STATE_MASK) |
 				  (lowerdev->state & MACVLAN_STATE_MASK);
 	dev->features 		= lowerdev->features & MACVLAN_FEATURES;
+	dev->features		|= NETIF_F_LLTX;
 	dev->gso_max_size	= lowerdev->gso_max_size;
 	dev->iflink		= lowerdev->ifindex;
 	dev->hard_header_len	= lowerdev->hard_header_len;
 
 	macvlan_set_lockdep_class(dev);
 
-	vlan->rx_stats = alloc_percpu(struct macvlan_rx_stats);
-	if (!vlan->rx_stats)
+	vlan->pcpu_stats = alloc_percpu(struct macvlan_pcpu_stats);
+	if (!vlan->pcpu_stats)
 		return -ENOMEM;
 
 	return 0;
@@ -431,7 +434,7 @@ static void macvlan_uninit(struct net_device *dev)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
 
-	free_percpu(vlan->rx_stats);
+	free_percpu(vlan->pcpu_stats);
 }
 
 static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
@@ -439,33 +442,34 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
 
-	dev_txq_stats_fold(dev, stats);
-
-	if (vlan->rx_stats) {
-		struct macvlan_rx_stats *p, accum = {0};
-		u64 rx_packets, rx_bytes, rx_multicast;
+	if (vlan->pcpu_stats) {
+		struct macvlan_pcpu_stats *p;
+		u64 rx_packets, rx_bytes, rx_multicast, tx_packets, tx_bytes;
 		unsigned int start;
 		int i;
 
 		for_each_possible_cpu(i) {
-			p = per_cpu_ptr(vlan->rx_stats, i);
+			p = per_cpu_ptr(vlan->pcpu_stats, i);
 			do {
 				start = u64_stats_fetch_begin_bh(&p->syncp);
 				rx_packets	= p->rx_packets;
 				rx_bytes	= p->rx_bytes;
 				rx_multicast	= p->rx_multicast;
+				tx_packets	= p->tx_packets;
+				tx_bytes	= p->tx_bytes;
 			} while (u64_stats_fetch_retry_bh(&p->syncp, start));
-			accum.rx_packets	+= rx_packets;
-			accum.rx_bytes		+= rx_bytes;
-			accum.rx_multicast	+= rx_multicast;
-			/* rx_errors is an ulong, updated without syncp protection */
-			accum.rx_errors		+= p->rx_errors;
+			stats->rx_packets	+= rx_packets;
+			stats->rx_bytes		+= rx_bytes;
+			stats->multicast	+= rx_multicast;
+			stats->tx_packets	+= tx_packets;
+			stats->tx_bytes		+= tx_bytes;
+			/* rx_errors & tx_dropped are ulong, updated
+			 * without syncp protection
+			 */
+			stats->rx_errors	+= p->rx_errors;
+			stats->tx_dropped	+= p->tx_dropped;
 		}
-		stats->rx_packets = accum.rx_packets;
-		stats->rx_bytes   = accum.rx_bytes;
-		stats->rx_errors  = accum.rx_errors;
-		stats->rx_dropped = accum.rx_errors;
-		stats->multicast  = accum.rx_multicast;
+		stats->rx_dropped = stats->rx_errors;
 	}
 	return stats;
 }
@@ -601,25 +605,6 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
-static int macvlan_get_tx_queues(struct net *net,
-				 struct nlattr *tb[],
-				 unsigned int *num_tx_queues,
-				 unsigned int *real_num_tx_queues)
-{
-	struct net_device *real_dev;
-
-	if (!tb[IFLA_LINK])
-		return -EINVAL;
-
-	real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
-	if (!real_dev)
-		return -ENODEV;
-
-	*num_tx_queues      = real_dev->num_tx_queues;
-	*real_num_tx_queues = real_dev->real_num_tx_queues;
-	return 0;
-}
-
 int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 			   struct nlattr *tb[], struct nlattr *data[],
 			   int (*receive)(struct sk_buff *skb),
@@ -743,7 +728,6 @@ int macvlan_link_register(struct rtnl_link_ops *ops)
 {
 	/* common fields */
 	ops->priv_size		= sizeof(struct macvlan_dev);
-	ops->get_tx_queues	= macvlan_get_tx_queues;
 	ops->validate		= macvlan_validate;
 	ops->maxtype		= IFLA_MACVLAN_MAX;
 	ops->policy		= macvlan_policy;
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
index 8a2fd66..3779b5f 100644
--- a/include/linux/if_macvlan.h
+++ b/include/linux/if_macvlan.h
@@ -25,19 +25,23 @@ struct macvlan_port;
 struct macvtap_queue;
 
 /**
- *	struct macvlan_rx_stats - MACVLAN percpu rx stats
+ *	struct macvlan_pcpu_stats - MACVLAN percpu stats
  *	@rx_packets: number of received packets
  *	@rx_bytes: number of received bytes
  *	@rx_multicast: number of received multicast packets
+ *	@tx_
  *	@syncp: synchronization point for 64bit counters
  *	@rx_errors: number of errors
  */
-struct macvlan_rx_stats {
+struct macvlan_pcpu_stats {
 	u64			rx_packets;
 	u64			rx_bytes;
 	u64			rx_multicast;
+	u64			tx_packets;
+	u64			tx_bytes;
 	struct u64_stats_sync	syncp;
 	unsigned long		rx_errors;
+	unsigned long		tx_dropped;
 };
 
 /*
@@ -52,7 +56,7 @@ struct macvlan_dev {
 	struct hlist_node	hlist;
 	struct macvlan_port	*port;
 	struct net_device	*lowerdev;
-	struct macvlan_rx_stats __percpu *rx_stats;
+	struct macvlan_pcpu_stats __percpu *pcpu_stats;
 	enum macvlan_mode	mode;
 	int (*receive)(struct sk_buff *skb);
 	int (*forward)(struct net_device *dev, struct sk_buff *skb);
@@ -64,18 +68,18 @@ static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
 				    unsigned int len, bool success,
 				    bool multicast)
 {
-	struct macvlan_rx_stats *rx_stats;
+	struct macvlan_pcpu_stats *pcpu_stats;
 
-	rx_stats = this_cpu_ptr(vlan->rx_stats);
+	pcpu_stats = this_cpu_ptr(vlan->pcpu_stats);
 	if (likely(success)) {
-		u64_stats_update_begin(&rx_stats->syncp);
-		rx_stats->rx_packets++;;
-		rx_stats->rx_bytes += len;
+		u64_stats_update_begin(&pcpu_stats->syncp);
+		pcpu_stats->rx_packets++;;
+		pcpu_stats->rx_bytes += len;
 		if (multicast)
-			rx_stats->rx_multicast++;
-		u64_stats_update_end(&rx_stats->syncp);
+			pcpu_stats->rx_multicast++;
+		u64_stats_update_end(&pcpu_stats->syncp);
 	} else {
-		rx_stats->rx_errors++;
+		pcpu_stats->rx_errors++;
 	}
 }
 

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/10/2010 07:41 AM, Eric Dumazet wrote:
> macvlan is a stacked device, like tunnels. We should use the lockless
> mechanism we are using in tunnels and loopback.
>
> This patch completely removes locking in TX path.
>
> tx stat counters are added into existing percpu stat structure, renamed
> from rx_stats to pcpu_stats.
>
> Note : this reverts commit 2c11455321f37 (macvlan: add multiqueue
> capability)
>
> Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com>
> ---
>   drivers/net/macvlan.c      |   72 +++++++++++++----------------------
>   include/linux/if_macvlan.h |   26 +++++++-----
>   2 files changed, 43 insertions(+), 55 deletions(-)
>

> diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
> index 8a2fd66..3779b5f 100644
> --- a/include/linux/if_macvlan.h
> +++ b/include/linux/if_macvlan.h
> @@ -25,19 +25,23 @@ struct macvlan_port;
>   struct macvtap_queue;
>
>   /**
> - *	struct macvlan_rx_stats - MACVLAN percpu rx stats
> + *	struct macvlan_pcpu_stats - MACVLAN percpu stats
>    *	@rx_packets: number of received packets
>    *	@rx_bytes: number of received bytes
>    *	@rx_multicast: number of received multicast packets
> + *	@tx_

Minor nit..seems you missed a few there?

>    *	@syncp: synchronization point for 64bit counters
>    *	@rx_errors: number of errors
>    */
> -struct macvlan_rx_stats {
> +struct macvlan_pcpu_stats {
>   	u64			rx_packets;
>   	u64			rx_bytes;
>   	u64			rx_multicast;
> +	u64			tx_packets;
> +	u64			tx_bytes;
>   	struct u64_stats_sync	syncp;
>   	unsigned long		rx_errors;
> +	unsigned long		tx_dropped;

Any reason to not also make those u64?


Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 09:39 -0800, Ben Greear a écrit :

> >   /**
> > - *	struct macvlan_rx_stats - MACVLAN percpu rx stats
> > + *	struct macvlan_pcpu_stats - MACVLAN percpu stats
> >    *	@rx_packets: number of received packets
> >    *	@rx_bytes: number of received bytes
> >    *	@rx_multicast: number of received multicast packets
> > + *	@tx_
> 
> Minor nit..seems you missed a few there?
> 

Arg... you're right !

> >    *	@syncp: synchronization point for 64bit counters
> >    *	@rx_errors: number of errors
> >    */
> > -struct macvlan_rx_stats {
> > +struct macvlan_pcpu_stats {
> >   	u64			rx_packets;
> >   	u64			rx_bytes;
> >   	u64			rx_multicast;
> > +	u64			tx_packets;
> > +	u64			tx_bytes;
> >   	struct u64_stats_sync	syncp;
> >   	unsigned long		rx_errors;
> > +	unsigned long		tx_dropped;
> 
> Any reason to not also make those u64?
> 

Well, they are supposed to be not incremented often, and they are packet
counts only, so a wrap around in less than 5 seconds is very unlikely.

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/10/2010 09:43 AM, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 09:39 -0800, Ben Greear a écrit :
>
>>>    /**
>>> - *	struct macvlan_rx_stats - MACVLAN percpu rx stats
>>> + *	struct macvlan_pcpu_stats - MACVLAN percpu stats
>>>     *	@rx_packets: number of received packets
>>>     *	@rx_bytes: number of received bytes
>>>     *	@rx_multicast: number of received multicast packets
>>> + *	@tx_
>>
>> Minor nit..seems you missed a few there?
>>
>
> Arg... you're right !
>
>>>     *	@syncp: synchronization point for 64bit counters
>>>     *	@rx_errors: number of errors
>>>     */
>>> -struct macvlan_rx_stats {
>>> +struct macvlan_pcpu_stats {
>>>    	u64			rx_packets;
>>>    	u64			rx_bytes;
>>>    	u64			rx_multicast;
>>> +	u64			tx_packets;
>>> +	u64			tx_bytes;
>>>    	struct u64_stats_sync	syncp;
>>>    	unsigned long		rx_errors;
>>> +	unsigned long		tx_dropped;
>>
>> Any reason to not also make those u64?
>>
>
> Well, they are supposed to be not incremented often, and they are packet
> counts only, so a wrap around in less than 5 seconds is very unlikely.

I agree, but if these can be read from user-space, it can be tricky to make
solid code to deal with wraps when the thing wrapping can be 32 or 64 bits,
depending on whether the kernel is compiled 32-bit or 64-bit.

So, my preference is to use u32 or u64 so there is no guesswork involved.

To be sure, this problem exists in lots of places already (/proc/net/dev comes to mind),
but the fewer places the better in my opinion.

That said, I don't feel too strongly about it, so if you want to keep these
stats as they are, I shall argue no more :)

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 09:53 -0800, Ben Greear a écrit :

> I agree, but if these can be read from user-space, it can be tricky to make
> solid code to deal with wraps when the thing wrapping can be 32 or 64 bits,
> depending on whether the kernel is compiled 32-bit or 64-bit.
> 
> So, my preference is to use u32 or u64 so there is no guesswork involved.
> 
> To be sure, this problem exists in lots of places already (/proc/net/dev comes to mind),
> but the fewer places the better in my opinion.
> 

On a 32bit kernel, very few devices provide 64bit counters, so an
application reading /proc/net/dev should be prepared to handle 32 or
64bit counter.

On a 64bit kernel, many devices still provide 32, 36, 40 bit counters
(hardware based). Same conclusion for userspace.

So really, an SNMP application must be able to cope with any counter
width.

As percpu data is going to hurt in the 4096 cpu cases, we should try to
not make percpu structures too big.

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/10/2010 10:18 AM, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 09:53 -0800, Ben Greear a écrit :
>
>> I agree, but if these can be read from user-space, it can be tricky to make
>> solid code to deal with wraps when the thing wrapping can be 32 or 64 bits,
>> depending on whether the kernel is compiled 32-bit or 64-bit.
>>
>> So, my preference is to use u32 or u64 so there is no guesswork involved.
>>
>> To be sure, this problem exists in lots of places already (/proc/net/dev comes to mind),
>> but the fewer places the better in my opinion.
>>
>
> On a 32bit kernel, very few devices provide 64bit counters, so an
> application reading /proc/net/dev should be prepared to handle 32 or
> 64bit counter.
>
> On a 64bit kernel, many devices still provide 32, 36, 40 bit counters
> (hardware based). Same conclusion for userspace.

In my opinion, the kernel and/or driver should deal with this such that
at worst the user has to deal with 32 v/s 64 bits based on whether the
kernel is compiled for 32 or 64 bit CPUs.  (Let the driver sample at
intervals needed to never wrap it's counters more than once and update
software stats of well-defined bit-width, and present those software
counters to users.

In practice, this seems to be the case, at least for the NICs I've used
(mostly Intel).  But, please don't propagate the idea that any width of
counters is OK to present to user-space:  It is completely unfair to
make app writers have to know the network driver and/or hardware quirks to
know how often it must sample stats.

> So really, an SNMP application must be able to cope with any counter
> width.
>
> As percpu data is going to hurt in the 4096 cpu cases, we should try to
> not make percpu structures too big.

Well, maybe using u32 would have positive benefits on 64-bit kernels then?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 10:40 -0800, Ben Greear a écrit :

> In my opinion, the kernel and/or driver should deal with this such that
> at worst the user has to deal with 32 v/s 64 bits based on whether the
> kernel is compiled for 32 or 64 bit CPUs.  (Let the driver sample at
> intervals needed to never wrap it's counters more than once and update
> software stats of well-defined bit-width, and present those software
> counters to users.
> 

How so ? Are you willing to provide patches for all network drivers ?

> In practice, this seems to be the case, at least for the NICs I've used
> (mostly Intel).  But, please don't propagate the idea that any width of
> counters is OK to present to user-space:  It is completely unfair to
> make app writers have to know the network driver and/or hardware quirks to
> know how often it must sample stats.
> 

I am sorry Ben, but /proc/net/dev doesnt publish each counter effective
width. Its unfair, but its like that.

An appplication must be able to cope for wrap arounds, running on a 32
or 64bit kernel. Our duty is to provide 64bit counters for high speed
interfaces where possible.
For a 10Mb adapter, there is no need, since a 32bit counter doesnt wrap
in less than one hour (RFC1902 suggestion)

As I said, many drivers counters are not 32bit or 64bit. I did many
driver get_stats() checks lately...

Why should we cap them to 32bit if they really are 36 or 40 bits ? 


> Well, maybe using u32 would have positive benefits on 64-bit kernels then?
> 

But we want to handle 40/100Gbps devices, and keep SNMP apps happy.

We really need 64bit for them, and MACVLAN might be used on top of such
devices.

Or are you suggesting using u32 instead of "unsigned long" for
rx_errors/tx_dropped ?

This would indeed save 8 bytes per cpu per macvlan.

Re: [PATCH] macvlan: lockless tx path

[Ben Hutchings]

On Wed, 2010-11-10 at 21:33 +0100, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 10:40 -0800, Ben Greear a écrit :
> 
> > In my opinion, the kernel and/or driver should deal with this such that
> > at worst the user has to deal with 32 v/s 64 bits based on whether the
> > kernel is compiled for 32 or 64 bit CPUs.  (Let the driver sample at
> > intervals needed to never wrap it's counters more than once and update
> > software stats of well-defined bit-width, and present those software
> > counters to users.
> > 
> 
> How so ? Are you willing to provide patches for all network drivers ?
> 
> > In practice, this seems to be the case, at least for the NICs I've used
> > (mostly Intel).  But, please don't propagate the idea that any width of
> > counters is OK to present to user-space:  It is completely unfair to
> > make app writers have to know the network driver and/or hardware quirks to
> > know how often it must sample stats.
> > 
> 
> I am sorry Ben, but /proc/net/dev doesnt publish each counter effective
> width. Its unfair, but its like that.
> 
> An appplication must be able to cope for wrap arounds, running on a 32
> or 64bit kernel. Our duty is to provide 64bit counters for high speed
> interfaces where possible.
> For a 10Mb adapter, there is no need, since a 32bit counter doesnt wrap
> in less than one hour (RFC1902 suggestion)
> 
> As I said, many drivers counters are not 32bit or 64bit. I did many
> driver get_stats() checks lately...
> 
> Why should we cap them to 32bit if they really are 36 or 40 bits ? 
[...]

Drivers should calculate differences and accumulate them in a 64-bit
counter.  (A lot of hardware has read-to-clear counters anyway, in which
case the driver *has* to accumulate the values it reads.)

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 21:04 +0000, Ben Hutchings a écrit :

> Drivers should calculate differences and accumulate them in a 64-bit
> counter.  (A lot of hardware has read-to-clear counters anyway, in which
> case the driver *has* to accumulate the values it reads.)

You are mistaken. These are _hardware_ counters. If they were software,
of course they would be 32 or 64 bit.

And doing the thing you describe in software is racy.
I tried to remove many races, not to add new ones.

Yes, some drivers read one hardware counter using two instructions, and
this is racy.

Re: [PATCH] macvlan: lockless tx path

[Ben Hutchings]

On Wed, 2010-11-10 at 22:12 +0100, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 21:04 +0000, Ben Hutchings a écrit :
> 
> > Drivers should calculate differences and accumulate them in a 64-bit
> > counter.  (A lot of hardware has read-to-clear counters anyway, in which
> > case the driver *has* to accumulate the values it reads.)
> 
> You are mistaken. These are _hardware_ counters. If they were software,
> of course they would be 32 or 64 bit.

Of course I understood that.

> And doing the thing you describe in software is racy.
> I tried to remove many races, not to add new ones.

If you do it in ndo_get_stats{,64} and don't use your own lock, yes.

> Yes, some drivers read one hardware counter using two instructions, and
> this is racy.

Most hardware which supports MMIO to multi-word counters has some kind
of latching scheme where you can read the words/registers in order and
get consistent values (within a single counter; consistency between
counters is another matter).  Obviously you have to use a lock to
serialise stats updates in this case - whether or not you maintain wider
software counters.

Oh, another problem with using register values directly is that
statistics are likely to be reset whenever the device is reconfigured in
the way that requires a hardware reset.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/10/2010 12:33 PM, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 10:40 -0800, Ben Greear a écrit :
>
>> In my opinion, the kernel and/or driver should deal with this such that
>> at worst the user has to deal with 32 v/s 64 bits based on whether the
>> kernel is compiled for 32 or 64 bit CPUs.  (Let the driver sample at
>> intervals needed to never wrap it's counters more than once and update
>> software stats of well-defined bit-width, and present those software
>> counters to users.
>>
>
> How so ? Are you willing to provide patches for all network drivers ?

I'm willing to attempt to fix something that I use and can test.

Either way, I think it's legitimate to document at least the desired
behaviour so that driver writers know what to aim for.

>> In practice, this seems to be the case, at least for the NICs I've used
>> (mostly Intel).  But, please don't propagate the idea that any width of
>> counters is OK to present to user-space:  It is completely unfair to
>> make app writers have to know the network driver and/or hardware quirks to
>> know how often it must sample stats.
>>
>
> I am sorry Ben, but /proc/net/dev doesnt publish each counter effective
> width. Its unfair, but its like that.
>
> An appplication must be able to cope for wrap arounds, running on a 32
> or 64bit kernel. Our duty is to provide 64bit counters for high speed
> interfaces where possible.
> For a 10Mb adapter, there is no need, since a 32bit counter doesnt wrap
> in less than one hour (RFC1902 suggestion)

So an application that must deal with wraps must poll at the minimal
time interval for wrapping 32-bit counters at whatever speed, or it
must pay attention to the driver to somehow know that this magic driver
can *really* do 64-bit stats properly?

Please note that just because a counter is less than the previous read,
that doesn't by itself tell us if it wrapped once or twice.  And, if we
don't know at which number of bits it wraps, then we don't know how many
to add even if we are certain it wrapped only once.

In general, I want to treat eth0 the same as eth5, and not worry that one
is 10/100 realtek and the other a 10G Intel.

If netlink reports stats64, then those should only wrap at 64 bits,
and if it reports stats32, then wrap at 32-bits.

> As I said, many drivers counters are not 32bit or 64bit. I did many
> driver get_stats() checks lately...
>
> Why should we cap them to 32bit if they really are 36 or 40 bits ?
>
>
>> Well, maybe using u32 would have positive benefits on 64-bit kernels then?
>>
>
> But we want to handle 40/100Gbps devices, and keep SNMP apps happy.
>
> We really need 64bit for them, and MACVLAN might be used on top of such
> devices.
>
> Or are you suggesting using u32 instead of "unsigned long" for
> rx_errors/tx_dropped ?
>
> This would indeed save 8 bytes per cpu per macvlan.

Yes, that was what I was trying to suggest.  I'm all for 64-bit numbers
in anything that can wrap anytime soon, and anywhere you think 32-bits
is enough, just use u32 so we don't have to worry about the number of
bits in 'unsigned long' on different platforms.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 13:35 -0800, Ben Greear a écrit :


> So an application that must deal with wraps must poll at the minimal
> time interval for wrapping 32-bit counters at whatever speed, or it
> must pay attention to the driver to somehow know that this magic driver
> can *really* do 64-bit stats properly?
> 

Are you aware that you speak of something that is not specified at all
in linux ?

Frequency of polling is not part of any RFC. This usually is tunable in
the _application_. Some people sample stats every 5 minutes, some sample
every second, and hit the "xxx driver updates its stats every two
seconds, this sucks"

I wrote SNMP apps based on /proc/net/dev and all just work, with any
versions, any driver. Of course, some of them broke 6 years ago because
they were 32bit legacy application, running on a 64bit kernel. I never
asked David to change /proc/net/dev to cap counters to 32bit.

When 128bit cpu come, some userland changes are needed to parse 128bit
numbers.

In anycase, apps dont have to know a particular driver provides 64bit or
32bit counter. Only choice for them is to automatically detect the
wraparound, because they fetch a STRING, not a Counter32 or Counter64

This works for all drivers, legacy, new, Intel or whatever. If a driver
changes from 32 to 64, nothing special happens in /proc/net/dev.

RRD for example handles this just fine.

> Please note that just because a counter is less than the previous read,
> that doesn't by itself tell us if it wrapped once or twice.  And, if we
> don't know at which number of bits it wraps, then we don't know how many
> to add even if we are certain it wrapped only once.
> 

I repeat : Nothing in /proc/net/dev can tell you when a counter will
wrap (the counter width).

You also need to use the correct polling frequency, depending on max
speed. It was already the case with 32bit counters, 64bit ones only gave
some extra range.

> In general, I want to treat eth0 the same as eth5, and not worry that one
> is 10/100 realtek and the other a 10G Intel.
> 


> If netlink reports stats64, then those should only wrap at 64 bits,
> and if it reports stats32, then wrap at 32-bits.
> 

I believe you are mistaken. We provide stats64 for all drivers, even
32bit legacy ones. rtnetlink has no way to report counter widths,
because nobody cared.

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/10/2010 02:21 PM, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 13:35 -0800, Ben Greear a écrit :

NOTE:  I trimmed the CC list..this has nothing in particular to
do with mac-vlans now.

>> So an application that must deal with wraps must poll at the minimal
>> time interval for wrapping 32-bit counters at whatever speed, or it
>> must pay attention to the driver to somehow know that this magic driver
>> can *really* do 64-bit stats properly?
>>
>
> Are you aware that you speak of something that is not specified at all
> in linux ?
>
> Frequency of polling is not part of any RFC. This usually is tunable in
> the _application_. Some people sample stats every 5 minutes, some sample
> every second, and hit the "xxx driver updates its stats every two
> seconds, this sucks"

These '2 sec' granularity bugs are a pain and should be (and have been)
fixed.

> I wrote SNMP apps based on /proc/net/dev and all just work, with any
> versions, any driver. Of course, some of them broke 6 years ago because
> they were 32bit legacy application, running on a 64bit kernel. I never
> asked David to change /proc/net/dev to cap counters to 32bit.

I did similar, and then wrote extra code to detect a 64-bit kernel and if
so assume that the counters wrap at 64 bits so I didn't have to poll so
often to make sure I didn't miss a wrap for a 10G NIC.  If instead one wraps at 33
bits and the other at 36, there is no way for me to deal with the wrap
properly w/out explicitly knowing about that 33 and 36.

If the old 32-bit counters in /proc/net/dev instead had a driver that
managed to wrap them at 28 bits, I can't see how your application could
have worked properly, so you must have been assuming that the kernel would
always return a full 32-bit counter.

Now, I'm trying to use netlink api since I'm hoping that is more efficient
and controllable than just reading /proc/net/dev over and over.  Netlink
explicitly can return a set of 32-bit counters or 64-bit.

All I want is to ensure than they are actually 32 or 64 bit, not 36 bit crammed in a
64-bit counter.  In other words, make the driver and/or kernel do it's
job and abstract access to hardware and return a consistent interface.

>> Please note that just because a counter is less than the previous read,
>> that doesn't by itself tell us if it wrapped once or twice.  And, if we
>> don't know at which number of bits it wraps, then we don't know how many
>> to add even if we are certain it wrapped only once.
>>
>
> I repeat : Nothing in /proc/net/dev can tell you when a counter will
> wrap (the counter width).

My primary concern is netlink API now, and even for proc/net/dev, there is no
good reason to show 32-bit counters mixed with 64-bit counters on 64-bit systems.
The kernel can deal with this easily enough, and it should.

>> If netlink reports stats64, then those should only wrap at 64 bits,
>> and if it reports stats32, then wrap at 32-bits.
>>
>
> I believe you are mistaken. We provide stats64 for all drivers, even
> 32bit legacy ones. rtnetlink has no way to report counter widths,
> because nobody cared.

Then it's busted.  If it claims to return stats64, but instead is returning
something different, it is wrong.  Netlink API still defines a stats32, so
the kernel should use that if it can't reliably deal with 64-bit counters.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 14:53 -0800, Ben Greear a écrit :

> Then it's busted.  If it claims to return stats64, but instead is returning
> something different, it is wrong.  Netlink API still defines a stats32, so
> the kernel should use that if it can't reliably deal with 64-bit counters.
> 

It claims nothing like that. You obviously assumed wrong things.

It provides a framework, not a mandatory or exclusive one.

Really this endless discussion is going nowhere.

I suggest you send patches if you want, I am very pleased by current
stats handling.

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 14:53 -0800, Ben Greear a écrit :

> I did similar, and then wrote extra code to detect a 64-bit kernel and if
> so assume that the counters wrap at 64 bits so I didn't have to poll so
> often to make sure I didn't miss a wrap for a 10G NIC.  If instead one wraps at 33
> bits and the other at 36, there is no way for me to deal with the wrap
> properly w/out explicitly knowing about that 33 and 36.
> 

How do you define 'wrap around' ? Maybe your definition is wrong.

> If the old 32-bit counters in /proc/net/dev instead had a driver that
> managed to wrap them at 28 bits, I can't see how your application could
> have worked properly, so you must have been assuming that the kernel would
> always return a full 32-bit counter.

I suggest you take a look at various SNMP applications that handle this
just fine. RRD for example. You can patch your favorite driver to cap
the stats to 37, 38, ... 31 or 30 bits, it works.

If you sample values faster than half the period, it works.

This has nothing to do with 32 or 64 bits, really.

Read RFC1230 for a good advice


	All the statistics are defined using the
        syntax Counter as 32 bit wrap around
        counters.  Thus, if an interface's
        hardware chip set maintains these
        statistics in 16-bit counters, then the
        agent must read the hardware's counters
        frequently enough to prevent loss of
        significance, in order to maintain
        a 32-bit counter in software."

Yes, 16bit counters _are_ fine, even if provided in a 32bit counter, if
you read value fast enough.

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/10/2010 03:36 PM, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 14:53 -0800, Ben Greear a écrit :
>
>> I did similar, and then wrote extra code to detect a 64-bit kernel and if
>> so assume that the counters wrap at 64 bits so I didn't have to poll so
>> often to make sure I didn't miss a wrap for a 10G NIC.  If instead one wraps at 33
>> bits and the other at 36, there is no way for me to deal with the wrap
>> properly w/out explicitly knowing about that 33 and 36.
>>
>
> How do you define 'wrap around' ? Maybe your definition is wrong.

Maybe so.  My algorithm looks like:

  // uint64 accum;
  // uint32 old;
  // uint32 new;
  if (old > new) {
      // This assumes counters wrap at 32 bits (ie, 0xFFFFFFFF).
      accum += ((uint32)(0xFFFFFFFF) - old) + new;
  }
  else if (old < new) {
      accum += new - old;
  }
  old = new;

...

Is there some way I can do this w/out the (0xFFFFFFFF - old),
and thus the assumption of 32-bit counters?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le mercredi 10 novembre 2010 à 15:46 -0800, Ben Greear a écrit :
> On 11/10/2010 03:36 PM, Eric Dumazet wrote:
> > Le mercredi 10 novembre 2010 à 14:53 -0800, Ben Greear a écrit :
> >
> >> I did similar, and then wrote extra code to detect a 64-bit kernel and if
> >> so assume that the counters wrap at 64 bits so I didn't have to poll so
> >> often to make sure I didn't miss a wrap for a 10G NIC.  If instead one wraps at 33
> >> bits and the other at 36, there is no way for me to deal with the wrap
> >> properly w/out explicitly knowing about that 33 and 36.
> >>
> >
> > How do you define 'wrap around' ? Maybe your definition is wrong.
> 
> Maybe so.  My algorithm looks like:
> 
>   // uint64 accum;
>   // uint32 old;
>   // uint32 new;
>   if (old > new) {
>       // This assumes counters wrap at 32 bits (ie, 0xFFFFFFFF).
>       accum += ((uint32)(0xFFFFFFFF) - old) + new;
>   }
>   else if (old < new) {
>       accum += new - old;
>   }
>   old = new;
> 
> ...
> 
> Is there some way I can do this w/out the (0xFFFFFFFF - old),
> and thus the assumption of 32-bit counters?
> 

Yes, please take a look at RRD for an example, then you can adapt to
your needs.

<quote>

http://www.mrtg.org/rrdtool/tut/rrdtutorial.en.html

At the time of writing this document, RRDtool knows of counters that are
either 32 bits or 64 bits of size. These counters can handle the
following different values:

 - 32 bits: 0 ..           4294967295
 - 64 bits: 0 .. 18446744073709551615

If these numbers look strange to you, you can view them in their
hexadecimal form:

 - 32 bits: 0 ..         FFFFFFFF
 - 64 bits: 0 .. FFFFFFFFFFFFFFFF

RRDtool handles both counters the same. If an overflow occurs and the
delta would be negative, RRDtool first adds the maximum of a small
counter + 1 to the delta. If the delta is still negative, it had to be
the large counter that wrapped. Add the maximum possible value of the
large counter + 1 and subtract the erroneously added small value.

There is a risk in this: suppose the large counter wrapped while adding
a huge delta, it could happen, theoretically, that adding the smaller
value would make the delta positive. In this unlikely case the results
would not be correct. The increase should be nearly as high as the
maximum counter value for that to happen, so chances are you would have
several other problems as well and this particular problem would not
even be worth thinking about. Even though, I did include an example, so
you can judge for yourself.

The next section gives you some numerical examples for counter-wraps.
Try to do the calculations yourself or just believe me if your
calculator can't handle the numbers :)

Correction numbers:

 - 32 bits: (4294967295 + 1) =                                4294967296
 - 64 bits: (18446744073709551615 + 1)
                                    - correction1 = 18446744069414584320

 Before:        4294967200
 Increase:                100
 Should become: 4294967300
 But really is:             4
 Delta:        -4294967196
 Correction1:  -4294967196 + 4294967296 = 100

 Before:        18446744073709551000
 Increase:                             800
 Should become: 18446744073709551800
 But really is:                        184
 Delta:        -18446744073709550816
 Correction1:  -18446744073709550816
                                + 4294967296 = -18446744069414583520
 Correction2:  -18446744069414583520
                   + 18446744069414584320 = 800

 Before:        18446744073709551615 ( maximum value )
 Increase:      18446744069414584320 ( absurd increase, minimum for
 Should become: 36893488143124135935             this example to work )
 But really is: 18446744069414584319
 Delta:                     -4294967296
 Correction1:  -4294967296 + 4294967296 = 0
 (not negative -> no correction2)

 Before:        18446744073709551615 ( maximum value )
 Increase:      18446744069414584319 ( one less increase )
 Should become: 36893488143124135934
 But really is: 18446744069414584318
 Delta:                     -4294967297
 Correction1:  -4294967297 + 4294967296 = -1
 Correction2:  -1 + 18446744069414584320 = 18446744069414584319

As you can see from the last two examples, you need strange numbers for
RRDtool to fail (provided it's bug free of course), so this should not
happen. However, SNMP or whatever method you choose to collect the data,
might also report wrong numbers occasionally. We can't prevent all
errors, but there are some things we can do. The RRDtool "create"
command takes two special parameters for this. They define the minimum
and maximum allowed values. Until now, we used "U", meaning "unknown".
If you provide values for one or both of them and if RRDtool receives
data points that are outside these limits, it will ignore those values.
For a thermometer in degrees Celsius, the absolute minimum is just under
-273. For my router, I can assume this minimum is much higher so I would
set it to 10, where as the maximum temperature I would set to 80. Any
higher and the device would be out of order.

For the speed of my car, I would never expect negative numbers and also
I would not expect a speed higher than 230. Anything else, and there
must have been an error. Remember: the opposite is not true, if the
numbers pass this check, it doesn't mean that they are correct. Always
judge the graph with a healthy dose of suspicion if it seems weird to
you.

http://www.mrtg.org/rrdtool/doc/rrdcreate.en.html

COUNTER

    is for continuous incrementing counters like the ifInOctets counter
in a router. The COUNTER data source assumes that the counter never
decreases, except when a counter overflows. The update function takes
the overflow into account. The counter is stored as a per-second rate.
When the counter overflows, RRDtool checks if the overflow happened at
the 32bit or 64bit border and acts accordingly by adding an appropriate
value to the result.

</quote>

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/10/2010 11:03 PM, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 15:46 -0800, Ben Greear a écrit :
>> On 11/10/2010 03:36 PM, Eric Dumazet wrote:
>>> Le mercredi 10 novembre 2010 à 14:53 -0800, Ben Greear a écrit :
>>>
>>>> I did similar, and then wrote extra code to detect a 64-bit kernel and if
>>>> so assume that the counters wrap at 64 bits so I didn't have to poll so
>>>> often to make sure I didn't miss a wrap for a 10G NIC.  If instead one wraps at 33
>>>> bits and the other at 36, there is no way for me to deal with the wrap
>>>> properly w/out explicitly knowing about that 33 and 36.
>>>>
>>>
>>> How do you define 'wrap around' ? Maybe your definition is wrong.
>>
>> Maybe so.  My algorithm looks like:
>>
>>    // uint64 accum;
>>    // uint32 old;
>>    // uint32 new;
>>    if (old>  new) {
>>        // This assumes counters wrap at 32 bits (ie, 0xFFFFFFFF).
>>        accum += ((uint32)(0xFFFFFFFF) - old) + new;
>>    }
>>    else if (old<  new) {
>>        accum += new - old;
>>    }
>>    old = new;
>>
>> ...
>>
>> Is there some way I can do this w/out the (0xFFFFFFFF - old),
>> and thus the assumption of 32-bit counters?
>>
>
> Yes, please take a look at RRD for an example, then you can adapt to
> your needs.
>
> <quote>
>
> http://www.mrtg.org/rrdtool/tut/rrdtutorial.en.html
>
> At the time of writing this document, RRDtool knows of counters that are
> either 32 bits or 64 bits of size. These counters can handle the
> following different values:
>
>   - 32 bits: 0 ..           4294967295
>   - 64 bits: 0 .. 18446744073709551615
>
> If these numbers look strange to you, you can view them in their
> hexadecimal form:
>
>   - 32 bits: 0 ..         FFFFFFFF
>   - 64 bits: 0 .. FFFFFFFFFFFFFFFF
>
> RRDtool handles both counters the same. If an overflow occurs and the
> delta would be negative, RRDtool first adds the maximum of a small
> counter + 1 to the delta. If the delta is still negative, it had to be
> the large counter that wrapped. Add the maximum possible value of the
> large counter + 1 and subtract the erroneously added small value.
>
> There is a risk in this: suppose the large counter wrapped while adding
> a huge delta, it could happen, theoretically, that adding the smaller
> value would make the delta positive. In this unlikely case the results
> would not be correct. The increase should be nearly as high as the
> maximum counter value for that to happen, so chances are you would have
> several other problems as well and this particular problem would not
> even be worth thinking about. Even though, I did include an example, so
> you can judge for yourself.

So, they assume counters are exactly 32 or 64 bits.
Your example of the 36-bit counter would break their
assumptions of 32 or 64 bits.

I agree that you can guess if the counter is 32 or 64, at least with today's
hardware and relatively normal poll times, and the requirement that the
counters can ONLY be 32 or 64 bits.  I still consider it a kludge to
return 32 bit counters in stats64, however.  Would you consider
a patch to have netlink pay attention to whether the stats are 32 or
64 (based on a flag returned from dev_get_stats perhaps)?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le jeudi 11 novembre 2010 à 08:40 -0800, Ben Greear a écrit :

> So, they assume counters are exactly 32 or 64 bits.
> Your example of the 36-bit counter would break their
> assumptions of 32 or 64 bits.
> 

They 'assume'. I am not. How do you handle counters that suddenly go to
0 ?

> I agree that you can guess if the counter is 32 or 64, at least with today's
> hardware and relatively normal poll times, and the requirement that the
> counters can ONLY be 32 or 64 bits.  I still consider it a kludge to
> return 32 bit counters in stats64, however.  Would you consider
> a patch to have netlink pay attention to whether the stats are 32 or
> 64 (based on a flag returned from dev_get_stats perhaps)?

So what ? How is it going to help /proc/net/dev users (most apps use it)

Could you please adapt your software, and not adapt linux to your
needs ? Dont your software runs on linux 2.6.32 ?

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/11/2010 08:56 AM, Eric Dumazet wrote:
> Le jeudi 11 novembre 2010 à 08:40 -0800, Ben Greear a écrit :
>
>> So, they assume counters are exactly 32 or 64 bits.
>> Your example of the 36-bit counter would break their
>> assumptions of 32 or 64 bits.
>>
>
> They 'assume'. I am not. How do you handle counters that suddenly go to
> 0 ?

I've shown you my algorithm that requires one to know the counter
width, and in response you offered on that will work with 32 OR 64 bits,
as long as you make some assumptions.

If you have an algorithm that can properly deal with wrapped counters of
arbitrary bits, then post it.

As for counters that go to zero, if the previous value was > 0, then
it was a wrap.  There is a reason Dave won't let anyone add a patch to
clear network counters.  If the network device was removed and came back,
then you must listen for those events and take proper precautions (set
prev to 0 before sampling any counters, for instance).


>> I agree that you can guess if the counter is 32 or 64, at least with today's
>> hardware and relatively normal poll times, and the requirement that the
>> counters can ONLY be 32 or 64 bits.  I still consider it a kludge to
>> return 32 bit counters in stats64, however.  Would you consider
>> a patch to have netlink pay attention to whether the stats are 32 or
>> 64 (based on a flag returned from dev_get_stats perhaps)?
>
> So what ? How is it going to help /proc/net/dev users (most apps use it)

At least they will have an opportunity to use a better defined API
if they wish.  And, if you only want stats for one interface, and
you have 4000 VLANs in the system, reading /proc/net/dev seems quite
inefficient.

> Could you please adapt your software, and not adapt linux to your
> needs ? Dont your software runs on linux 2.6.32 ?

Yes, I used to carry a patch that allowed direct access to the netdev_stats,
and I'd fall back to parsing /proc/net/dev on standard kernels.  I've now
moved to using netlink API as that seems the preferred method going
forward and allows the granularity (ie, get stats for a single device)
that I prefer.

And, I'll certainly keep trying to improve Linux, because if it helps
my needs, it may help others.  If it causes harm to others, then hopefully
someone will notice and reject my patch or help me to see how to make
it better.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le jeudi 11 novembre 2010 à 09:20 -0800, Ben Greear a écrit :

> I've shown you my algorithm that requires one to know the counter
> width, and in response you offered on that will work with 32 OR 64 bits,
> as long as you make some assumptions.
> 
> If you have an algorithm that can properly deal with wrapped counters of
> arbitrary bits, then post it.
> 

Its only a generalization of RRD algo

No rocket science I am afraid.

You can try all the numbers in `seq 32 64` in this order, to get a
generic algo.

If you can certify all your devices are 32 or 64, then the RRD method is
OK.

Re: [PATCH] macvlan: lockless tx path

[Ben Greear]

On 11/11/2010 10:02 AM, Eric Dumazet wrote:
> Le jeudi 11 novembre 2010 à 09:20 -0800, Ben Greear a écrit :
>
>> I've shown you my algorithm that requires one to know the counter
>> width, and in response you offered on that will work with 32 OR 64 bits,
>> as long as you make some assumptions.
>>
>> If you have an algorithm that can properly deal with wrapped counters of
>> arbitrary bits, then post it.
>>
>
> Its only a generalization of RRD algo
>
> No rocket science I am afraid.
>
> You can try all the numbers in `seq 32 64` in this order, to get a
> generic algo.

Ok, so then you have to sample soon enough that a 32-bit counter
can't wrap twice..otherwise you couldn't tell a 32 bit from a
33 bit counter, and you basically gain nothing from having "64-bit"
stats.

And that still assumes at least 32-bit stats..not 16 or whatever.
Thankfully, I doubt there are any drivers using < 32 bit stats.

I'll work on a patch for my idea when I get a chance..we'll see if
anyone likes it.

If you are aware of any drivers that return counters of other than 32 or
64bit widths, please let us know and perhaps we can fix them as well.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: lockless tx path

[Eric Dumazet]

Le jeudi 11 novembre 2010 à 10:13 -0800, Ben Greear a écrit :

> If you are aware of any drivers that return counters of other than 32 or
> 64bit widths, please let us know and perhaps we can fix them as well.

I am pretty sure I found some of them, but cannot recall right now.

Some ethtool stats have definitly not a 32 or 64 bit range

[PATCH net-next-2.6 v2] macvlan: lockless tx path

[Eric Dumazet]

macvlan is a stacked device, like tunnels. We should use the lockless
mechanism we are using in tunnels and loopback.

This patch completely removes locking in TX path.

tx stat counters are added into existing percpu stat structure, renamed
from rx_stats to pcpu_stats.

Note : this reverts commit 2c11455321f37 (macvlan: add multiqueue
capability)

Note : rx_errors converted to a 32bit counter, like tx_dropped, since
they dont need 64bit range.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Ben Greear <greearb@candelatech.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
V2: correct kerneldoc
    u32 for tx_dropped and rx_errors

 drivers/net/macvlan.c      |   80 +++++++++++++++--------------------
 include/linux/if_macvlan.h |   34 ++++++++------
 2 files changed, 55 insertions(+), 59 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 0fc9dc7..93f0ba2 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -243,18 +243,22 @@ xmit_world:
 netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
 			       struct net_device *dev)
 {
-	int i = skb_get_queue_mapping(skb);
-	struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
 	unsigned int len = skb->len;
 	int ret;
+	const struct macvlan_dev *vlan = netdev_priv(dev);
 
 	ret = macvlan_queue_xmit(skb, dev);
 	if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) {
-		txq->tx_packets++;
-		txq->tx_bytes += len;
-	} else
-		txq->tx_dropped++;
+		struct macvlan_pcpu_stats *pcpu_stats;
 
+		pcpu_stats = this_cpu_ptr(vlan->pcpu_stats);
+		u64_stats_update_begin(&pcpu_stats->syncp);
+		pcpu_stats->tx_packets++;
+		pcpu_stats->tx_bytes += len;
+		u64_stats_update_end(&pcpu_stats->syncp);
+	} else {
+		this_cpu_inc(vlan->pcpu_stats->tx_dropped);
+	}
 	return ret;
 }
 EXPORT_SYMBOL_GPL(macvlan_start_xmit);
@@ -414,14 +418,15 @@ static int macvlan_init(struct net_device *dev)
 	dev->state		= (dev->state & ~MACVLAN_STATE_MASK) |
 				  (lowerdev->state & MACVLAN_STATE_MASK);
 	dev->features 		= lowerdev->features & MACVLAN_FEATURES;
+	dev->features		|= NETIF_F_LLTX;
 	dev->gso_max_size	= lowerdev->gso_max_size;
 	dev->iflink		= lowerdev->ifindex;
 	dev->hard_header_len	= lowerdev->hard_header_len;
 
 	macvlan_set_lockdep_class(dev);
 
-	vlan->rx_stats = alloc_percpu(struct macvlan_rx_stats);
-	if (!vlan->rx_stats)
+	vlan->pcpu_stats = alloc_percpu(struct macvlan_pcpu_stats);
+	if (!vlan->pcpu_stats)
 		return -ENOMEM;
 
 	return 0;
@@ -431,7 +436,7 @@ static void macvlan_uninit(struct net_device *dev)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
 
-	free_percpu(vlan->rx_stats);
+	free_percpu(vlan->pcpu_stats);
 }
 
 static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
@@ -439,33 +444,38 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
 
-	dev_txq_stats_fold(dev, stats);
-
-	if (vlan->rx_stats) {
-		struct macvlan_rx_stats *p, accum = {0};
-		u64 rx_packets, rx_bytes, rx_multicast;
+	if (vlan->pcpu_stats) {
+		struct macvlan_pcpu_stats *p;
+		u64 rx_packets, rx_bytes, rx_multicast, tx_packets, tx_bytes;
+		u32 rx_errors = 0, tx_dropped = 0;
 		unsigned int start;
 		int i;
 
 		for_each_possible_cpu(i) {
-			p = per_cpu_ptr(vlan->rx_stats, i);
+			p = per_cpu_ptr(vlan->pcpu_stats, i);
 			do {
 				start = u64_stats_fetch_begin_bh(&p->syncp);
 				rx_packets	= p->rx_packets;
 				rx_bytes	= p->rx_bytes;
 				rx_multicast	= p->rx_multicast;
+				tx_packets	= p->tx_packets;
+				tx_bytes	= p->tx_bytes;
 			} while (u64_stats_fetch_retry_bh(&p->syncp, start));
-			accum.rx_packets	+= rx_packets;
-			accum.rx_bytes		+= rx_bytes;
-			accum.rx_multicast	+= rx_multicast;
-			/* rx_errors is an ulong, updated without syncp protection */
-			accum.rx_errors		+= p->rx_errors;
+
+			stats->rx_packets	+= rx_packets;
+			stats->rx_bytes		+= rx_bytes;
+			stats->multicast	+= rx_multicast;
+			stats->tx_packets	+= tx_packets;
+			stats->tx_bytes		+= tx_bytes;
+			/* rx_errors & tx_dropped are u32, updated
+			 * without syncp protection.
+			 */
+			rx_errors	+= p->rx_errors;
+			tx_dropped	+= p->tx_dropped;
 		}
-		stats->rx_packets = accum.rx_packets;
-		stats->rx_bytes   = accum.rx_bytes;
-		stats->rx_errors  = accum.rx_errors;
-		stats->rx_dropped = accum.rx_errors;
-		stats->multicast  = accum.rx_multicast;
+		stats->rx_errors	= rx_errors;
+		stats->rx_dropped	= rx_errors;
+		stats->tx_dropped	= tx_dropped;
 	}
 	return stats;
 }
@@ -601,25 +611,6 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
-static int macvlan_get_tx_queues(struct net *net,
-				 struct nlattr *tb[],
-				 unsigned int *num_tx_queues,
-				 unsigned int *real_num_tx_queues)
-{
-	struct net_device *real_dev;
-
-	if (!tb[IFLA_LINK])
-		return -EINVAL;
-
-	real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
-	if (!real_dev)
-		return -ENODEV;
-
-	*num_tx_queues      = real_dev->num_tx_queues;
-	*real_num_tx_queues = real_dev->real_num_tx_queues;
-	return 0;
-}
-
 int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 			   struct nlattr *tb[], struct nlattr *data[],
 			   int (*receive)(struct sk_buff *skb),
@@ -743,7 +734,6 @@ int macvlan_link_register(struct rtnl_link_ops *ops)
 {
 	/* common fields */
 	ops->priv_size		= sizeof(struct macvlan_dev);
-	ops->get_tx_queues	= macvlan_get_tx_queues;
 	ops->validate		= macvlan_validate;
 	ops->maxtype		= IFLA_MACVLAN_MAX;
 	ops->policy		= macvlan_policy;
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
index 8a2fd66..e28b2e4 100644
--- a/include/linux/if_macvlan.h
+++ b/include/linux/if_macvlan.h
@@ -25,19 +25,25 @@ struct macvlan_port;
 struct macvtap_queue;
 
 /**
- *	struct macvlan_rx_stats - MACVLAN percpu rx stats
+ *	struct macvlan_pcpu_stats - MACVLAN percpu stats
  *	@rx_packets: number of received packets
  *	@rx_bytes: number of received bytes
  *	@rx_multicast: number of received multicast packets
+ *	@tx_packets: number of transmitted packets
+ *	@tx_bytes: number of transmitted bytes
  *	@syncp: synchronization point for 64bit counters
- *	@rx_errors: number of errors
+ *	@rx_errors: number of rx errors
+ *	@tx_dropped: number of tx dropped packets
  */
-struct macvlan_rx_stats {
+struct macvlan_pcpu_stats {
 	u64			rx_packets;
 	u64			rx_bytes;
 	u64			rx_multicast;
+	u64			tx_packets;
+	u64			tx_bytes;
 	struct u64_stats_sync	syncp;
-	unsigned long		rx_errors;
+	u32			rx_errors;
+	u32			tx_dropped;
 };
 
 /*
@@ -52,7 +58,7 @@ struct macvlan_dev {
 	struct hlist_node	hlist;
 	struct macvlan_port	*port;
 	struct net_device	*lowerdev;
-	struct macvlan_rx_stats __percpu *rx_stats;
+	struct macvlan_pcpu_stats __percpu *pcpu_stats;
 	enum macvlan_mode	mode;
 	int (*receive)(struct sk_buff *skb);
 	int (*forward)(struct net_device *dev, struct sk_buff *skb);
@@ -64,18 +70,18 @@ static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
 				    unsigned int len, bool success,
 				    bool multicast)
 {
-	struct macvlan_rx_stats *rx_stats;
-
-	rx_stats = this_cpu_ptr(vlan->rx_stats);
 	if (likely(success)) {
-		u64_stats_update_begin(&rx_stats->syncp);
-		rx_stats->rx_packets++;;
-		rx_stats->rx_bytes += len;
+		struct macvlan_pcpu_stats *pcpu_stats;
+
+		pcpu_stats = this_cpu_ptr(vlan->pcpu_stats);
+		u64_stats_update_begin(&pcpu_stats->syncp);
+		pcpu_stats->rx_packets++;
+		pcpu_stats->rx_bytes += len;
 		if (multicast)
-			rx_stats->rx_multicast++;
-		u64_stats_update_end(&rx_stats->syncp);
+			pcpu_stats->rx_multicast++;
+		u64_stats_update_end(&pcpu_stats->syncp);
 	} else {
-		rx_stats->rx_errors++;
+		this_cpu_inc(vlan->pcpu_stats->rx_errors);
 	}
 }
 

Re: [PATCH net-next-2.6 v2] macvlan: lockless tx path

[Patrick McHardy]

On 11.11.2010 08:14, Eric Dumazet wrote:
> macvlan is a stacked device, like tunnels. We should use the lockless
> mechanism we are using in tunnels and loopback.
> 
> This patch completely removes locking in TX path.
> 
> tx stat counters are added into existing percpu stat structure, renamed
> from rx_stats to pcpu_stats.
> 
> Note : this reverts commit 2c11455321f37 (macvlan: add multiqueue
> capability)
> 
> Note : rx_errors converted to a 32bit counter, like tx_dropped, since
> they dont need 64bit range.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Ben Greear <greearb@candelatech.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> ---
> V2: correct kerneldoc
>     u32 for tx_dropped and rx_errors

Looks good to me.

Acked-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH net-next-2.6 v2] macvlan: lockless tx path

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 12 Nov 2010 09:20:19 +0100

> On 11.11.2010 08:14, Eric Dumazet wrote:
>> macvlan is a stacked device, like tunnels. We should use the lockless
>> mechanism we are using in tunnels and loopback.
>> 
>> This patch completely removes locking in TX path.
>> 
>> tx stat counters are added into existing percpu stat structure, renamed
>> from rx_stats to pcpu_stats.
>> 
>> Note : this reverts commit 2c11455321f37 (macvlan: add multiqueue
>> capability)
>> 
>> Note : rx_errors converted to a 32bit counter, like tx_dropped, since
>> they dont need 64bit range.
>> 
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Cc: Patrick McHardy <kaber@trash.net>
>> Cc: Ben Greear <greearb@candelatech.com>
>> Cc: Ben Hutchings <bhutchings@solarflare.com>
>> ---
>> V2: correct kerneldoc
>>     u32 for tx_dropped and rx_errors
> 
> Looks good to me.
> 
> Acked-by: Patrick McHardy <kaber@trash.net>

Applied, thanks everyone.