From: "Michael S. Tsirkin"
Date: Sun, 10 Jun 2012 13:25:12 +0300
> On Wed, Jun 06, 2012 at 10:35:24AM +0200, Eric Dumazet wrote:
>> From: Eric Dumazet
>>
>> commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
>> on 32bit arches.
>>
>> We must use separate syncp for rx and t
On Wed, Jun 06, 2012 at 10:35:24AM +0200, Eric Dumazet wrote:
> From: Eric Dumazet
>
> commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
> on 32bit arches.
>
> We must use separate syncp for rx and tx path as they can be run at the
> same time on different cpus. Thus one se
On Sun, Jun 10, 2012 at 12:21:14PM +0200, Eric Dumazet wrote:
> On Sun, 2012-06-10 at 10:03 +0300, Michael S. Tsirkin wrote:
>
> > One question though: do we want to lay the structure
> > out so that the rx sync structure precedes the rx counters?
> >
>
> I am not sure its worth having holes in
On Sun, 2012-06-10 at 10:03 +0300, Michael S. Tsirkin wrote:
> One question though: do we want to lay the structure
> out so that the rx sync structure precedes the rx counters?
>
I am not sure its worth having holes in the structure, since its percpu
data.
That would be 8 bytes lost per cpu an
On Sun, Jun 10, 2012 at 04:06:34PM +0930, Rusty Russell wrote:
> On Wed, 06 Jun 2012 10:45:41 +0200, Eric Dumazet
> wrote:
> > On Wed, 2012-06-06 at 10:35 +0200, Eric Dumazet wrote:
> > > From: Eric Dumazet
> > >
> > > commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
> >
On Wed, 06 Jun 2012 10:45:41 +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 10:35 +0200, Eric Dumazet wrote:
> > From: Eric Dumazet
> >
> > commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
> > on 32bit arches.
> >
> > We must use separate syncp for rx and tx path as t
On Wed, Jun 06, 2012 at 09:35:04PM +0100, Ben Hutchings wrote:
> On Wed, 2012-06-06 at 23:16 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2012 at 10:08:09PM +0200, Eric Dumazet wrote:
> > > On Wed, 2012-06-06 at 22:58 +0300, Michael S. Tsirkin wrote:
> > >
> > > > Absolutely, I am talking
On Wed, 2012-06-06 at 22:24 +0200, Eric Dumazet wrote:
> (ndo_get_stats64() is not allowed to sleep, and I cant see how you are
> going to disable napi without sleeping)
>
>
In case you wonder, take a look at bond_get_stats() in
drivers/net/bonding/bond_main.c
___
On Wed, 2012-06-06 at 23:16 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2012 at 10:08:09PM +0200, Eric Dumazet wrote:
> > On Wed, 2012-06-06 at 22:58 +0300, Michael S. Tsirkin wrote:
> >
> > > Absolutely, I am talking about virtio here. I'm not kicking
> > > u64_stats_sync idea I am just s
On Wed, 2012-06-06 at 21:19 +0100, Ben Hutchings wrote:
> On Wed, 2012-06-06 at 22:08 +0200, Eric Dumazet wrote:
> > On Wed, 2012-06-06 at 22:58 +0300, Michael S. Tsirkin wrote:
> >
> > > Absolutely, I am talking about virtio here. I'm not kicking
> > > u64_stats_sync idea I am just saying that s
On Wed, 2012-06-06 at 22:08 +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 22:58 +0300, Michael S. Tsirkin wrote:
>
> > Absolutely, I am talking about virtio here. I'm not kicking
> > u64_stats_sync idea I am just saying that simple locking
> > would work for virtio and might be better as it
On Wed, 2012-06-06 at 23:16 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2012 at 10:08:09PM +0200, Eric Dumazet wrote:
> > On Wed, 2012-06-06 at 22:58 +0300, Michael S. Tsirkin wrote:
> >
> > > Absolutely, I am talking about virtio here. I'm not kicking
> > > u64_stats_sync idea I am just s
On Wed, Jun 06, 2012 at 10:06:11PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 21:43 +0300, Michael S. Tsirkin wrote:
>
> > 1. We are trying to look at counters for purposes of tuning the device.
> > E.g. if ethtool reports packets and bytes, we'd like to calculate
> > average packet size b
On Wed, Jun 06, 2012 at 10:08:09PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 22:58 +0300, Michael S. Tsirkin wrote:
>
> > Absolutely, I am talking about virtio here. I'm not kicking
> > u64_stats_sync idea I am just saying that simple locking
> > would work for virtio and might be better
On Wed, 2012-06-06 at 22:58 +0300, Michael S. Tsirkin wrote:
> Absolutely, I am talking about virtio here. I'm not kicking
> u64_stats_sync idea I am just saying that simple locking
> would work for virtio and might be better as it
> gives us a way to get counters atomically.
Which lock do you o
On Wed, 2012-06-06 at 21:43 +0300, Michael S. Tsirkin wrote:
> 1. We are trying to look at counters for purposes of tuning the device.
> E.g. if ethtool reports packets and bytes, we'd like to calculate
> average packet size by bytes/packets.
>
> If both counters are read atomically the metric be
On Wed, 2012-06-06 at 19:57 +0300, Michael S. Tsirkin wrote:
> So for virtio since all counters get incremented from bh we can
> ensure they are read atomically, simply but reading them
> from the correct CPU with bh disabled.
> And then we don't need u64_stats_sync at all.
>
Really ? How are yo
On Wed, Jun 06, 2012 at 09:54:01PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 21:51 +0300, Michael S. Tsirkin wrote:
>
> > BTW for cards that do implement the counters in software,
> > under xmit lock, is anything wrong with simply taking the xmit lock
> > when we get the stats instead of
On Wed, 2012-06-06 at 21:51 +0300, Michael S. Tsirkin wrote:
> BTW for cards that do implement the counters in software,
> under xmit lock, is anything wrong with simply taking the xmit lock
> when we get the stats instead of the per-cpu trick + seqlock?
>
I still dont understand why you would d
On Wed, Jun 06, 2012 at 05:19:04PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 17:49 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2012 at 03:10:10PM +0200, Eric Dumazet wrote:
> > > On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
> > >
> > > > We currently do all stats
On Wed, Jun 06, 2012 at 08:14:32AM -0700, Stephen Hemminger wrote:
> On Wed, 6 Jun 2012 17:49:42 +0300
> "Michael S. Tsirkin" wrote:
>
> > Sounds good, but I have a question: this realies on counters
> > being atomic on 64 bit.
> > Would not it be better to always use a seqlock even on 64 bit?
>
On Wed, Jun 06, 2012 at 07:13:02PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 19:17 +0300, Michael S. Tsirkin wrote:
>
> > But why do you say at most 1 packet?
> >
> > Consider get_stats doing:
> >u64_stats_update_begin(&stats->syncp);
> >stats->tx_bytes +=
On Wed, 2012-06-06 at 19:17 +0300, Michael S. Tsirkin wrote:
> But why do you say at most 1 packet?
>
> Consider get_stats doing:
>u64_stats_update_begin(&stats->syncp);
>stats->tx_bytes += skb->len;
>
> on 64 bit at this point
> tx_packets might get incremented a
On Wed, Jun 06, 2012 at 05:19:04PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 17:49 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2012 at 03:10:10PM +0200, Eric Dumazet wrote:
> > > On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
> > >
> > > > We currently do all stats
On Wed, 2012-06-06 at 17:49 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2012 at 03:10:10PM +0200, Eric Dumazet wrote:
> > On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
> >
> > > We currently do all stats either on napi callback or from
> > > start_xmit callback.
> > > This ma
On Wed, 6 Jun 2012 17:49:42 +0300
"Michael S. Tsirkin" wrote:
> Sounds good, but I have a question: this realies on counters
> being atomic on 64 bit.
> Would not it be better to always use a seqlock even on 64 bit?
> This way counters would actually be correct and in sync.
> As it is if we want
On Wed, Jun 06, 2012 at 03:10:10PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
>
> > We currently do all stats either on napi callback or from
> > start_xmit callback.
> > This makes them safe, yes?
>
> Hmm, then _bh() variant is needed in virtnet_sta
On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
> We currently do all stats either on napi callback or from
> start_xmit callback.
> This makes them safe, yes?
Hmm, then _bh() variant is needed in virtnet_stats(), as explained in
include/linux/u64_stats_sync.h section 6)
* 6) If co
On Wed, Jun 06, 2012 at 10:45:41AM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 10:35 +0200, Eric Dumazet wrote:
> > From: Eric Dumazet
> >
> > commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
> > on 32bit arches.
> >
> > We must use separate syncp for rx and tx path
On 06/06/2012 04:45 PM, Eric Dumazet wrote:
On Wed, 2012-06-06 at 10:35 +0200, Eric Dumazet wrote:
From: Eric Dumazet
commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
on 32bit arches.
We must use separate syncp for rx and tx path as they can be run at the
same time on di
On Wed, 2012-06-06 at 10:35 +0200, Eric Dumazet wrote:
> From: Eric Dumazet
>
> commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
> on 32bit arches.
>
> We must use separate syncp for rx and tx path as they can be run at the
> same time on different cpus. Thus one sequence
From: Eric Dumazet
commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
on 32bit arches.
We must use separate syncp for rx and tx path as they can be run at the
same time on different cpus. Thus one sequence increment can be lost and
readers spin forever.
Signed-off-by: Eric
32 matches
Mail list logo
