[RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()

[Jason Wang]

Accumulate the sent packets and sent bytes in local variables and perform a
single u64_stats_update_begin/end() after.

Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
 drivers/net/virtio_net.c |   12 
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3d0ce44..a4d56b8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -833,17 +833,21 @@ static void free_old_xmit_skbs(struct send_queue *sq)
unsigned int len;
struct virtnet_info *vi = sq-vq-vdev-priv;
struct virtnet_stats *stats = this_cpu_ptr(vi-stats);
+   u64 tx_bytes = 0, tx_packets = 0;
 
while ((skb = virtqueue_get_buf(sq-vq, len)) != NULL) {
pr_debug(Sent skb %p\n, skb);
 
-   u64_stats_update_begin(stats-tx_syncp);
-   stats-tx_bytes += skb-len;
-   stats-tx_packets++;
-   u64_stats_update_end(stats-tx_syncp);
+   tx_bytes += skb-len;
+   tx_packets++;
 
dev_kfree_skb_any(skb);
}
+
+   u64_stats_update_begin(stats-tx_syncp);
+   stats-tx_bytes += tx_bytes;
+   stats-tx_packets =+ tx_packets;
+   u64_stats_update_end(stats-tx_syncp);
 }
 
 static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
-- 
1.7.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()

[Michael S. Tsirkin]

On Wed, Oct 15, 2014 at 03:25:27PM +0800, Jason Wang wrote:
 Accumulate the sent packets and sent bytes in local variables and perform a
 single u64_stats_update_begin/end() after.
 
 Cc: Rusty Russell ru...@rustcorp.com.au
 Cc: Michael S. Tsirkin m...@redhat.com
 Signed-off-by: Jason Wang jasow...@redhat.com

Not sure how much it's worth but since Eric suggested it ...

Acked-by: Michael S. Tsirkin m...@redhat.com

 ---
  drivers/net/virtio_net.c |   12 
  1 files changed, 8 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
 index 3d0ce44..a4d56b8 100644
 --- a/drivers/net/virtio_net.c
 +++ b/drivers/net/virtio_net.c
 @@ -833,17 +833,21 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   unsigned int len;
   struct virtnet_info *vi = sq-vq-vdev-priv;
   struct virtnet_stats *stats = this_cpu_ptr(vi-stats);
 + u64 tx_bytes = 0, tx_packets = 0;
  
   while ((skb = virtqueue_get_buf(sq-vq, len)) != NULL) {
   pr_debug(Sent skb %p\n, skb);
  
 - u64_stats_update_begin(stats-tx_syncp);
 - stats-tx_bytes += skb-len;
 - stats-tx_packets++;
 - u64_stats_update_end(stats-tx_syncp);
 + tx_bytes += skb-len;
 + tx_packets++;
  
   dev_kfree_skb_any(skb);
   }
 +
 + u64_stats_update_begin(stats-tx_syncp);
 + stats-tx_bytes += tx_bytes;
 + stats-tx_packets =+ tx_packets;
 + u64_stats_update_end(stats-tx_syncp);
  }
  
  static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 -- 
 1.7.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()

[Eric Dumazet]

On Wed, 2014-10-15 at 15:25 +0800, Jason Wang wrote:
 Accumulate the sent packets and sent bytes in local variables and perform a
 single u64_stats_update_begin/end() after.
 
 Cc: Rusty Russell ru...@rustcorp.com.au
 Cc: Michael S. Tsirkin m...@redhat.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
  drivers/net/virtio_net.c |   12 
  1 files changed, 8 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
 index 3d0ce44..a4d56b8 100644
 --- a/drivers/net/virtio_net.c
 +++ b/drivers/net/virtio_net.c
 @@ -833,17 +833,21 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   unsigned int len;
   struct virtnet_info *vi = sq-vq-vdev-priv;
   struct virtnet_stats *stats = this_cpu_ptr(vi-stats);
 + u64 tx_bytes = 0, tx_packets = 0;
  
   while ((skb = virtqueue_get_buf(sq-vq, len)) != NULL) {
   pr_debug(Sent skb %p\n, skb);
  
 - u64_stats_update_begin(stats-tx_syncp);
 - stats-tx_bytes += skb-len;
 - stats-tx_packets++;
 - u64_stats_update_end(stats-tx_syncp);
 + tx_bytes += skb-len;
 + tx_packets++;
  
   dev_kfree_skb_any(skb);
   }
 +
 + u64_stats_update_begin(stats-tx_syncp);
 + stats-tx_bytes += tx_bytes;
 + stats-tx_packets =+ tx_packets;


stats-tx_packets += tx_packets;


 + u64_stats_update_end(stats-tx_syncp);
  }
  
  static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()

[Michael S. Tsirkin]

On Wed, Oct 15, 2014 at 09:49:01AM +, David Laight wrote:
 From: Of Michael S. Tsirkin
  On Wed, Oct 15, 2014 at 03:25:27PM +0800, Jason Wang wrote:
   Accumulate the sent packets and sent bytes in local variables and perform 
   a
   single u64_stats_update_begin/end() after.
  
   Cc: Rusty Russell ru...@rustcorp.com.au
   Cc: Michael S. Tsirkin m...@redhat.com
   Signed-off-by: Jason Wang jasow...@redhat.com
  
  Not sure how much it's worth but since Eric suggested it ...
 
 Probably depends on the actual cost of u64_stats_update_begin/end
 against the likely extra saving of the tx_bytes and tx_packets
 values onto the stack across the call to dev_kfree_skb_any().
 (Which depends on the number of caller saved registers.)

Yea, some benchmark results would be nice to see.


  Acked-by: Michael S. Tsirkin m...@redhat.com
  
   ---
drivers/net/virtio_net.c |   12 
1 files changed, 8 insertions(+), 4 deletions(-)
  
   diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
   index 3d0ce44..a4d56b8 100644
   --- a/drivers/net/virtio_net.c
   +++ b/drivers/net/virtio_net.c
   @@ -833,17 +833,21 @@ static void free_old_xmit_skbs(struct send_queue 
   *sq)
 unsigned int len;
 struct virtnet_info *vi = sq-vq-vdev-priv;
 struct virtnet_stats *stats = this_cpu_ptr(vi-stats);
   + u64 tx_bytes = 0, tx_packets = 0;
 
 tx_packets need only be 'unsigned int'.
 The same is almost certainly true of tx_bytes.
 
   David
 
 while ((skb = virtqueue_get_buf(sq-vq, len)) != NULL) {
 pr_debug(Sent skb %p\n, skb);
  
   - u64_stats_update_begin(stats-tx_syncp);
   - stats-tx_bytes += skb-len;
   - stats-tx_packets++;
   - u64_stats_update_end(stats-tx_syncp);
   + tx_bytes += skb-len;
   + tx_packets++;
  
 dev_kfree_skb_any(skb);
 }
   +
   + u64_stats_update_begin(stats-tx_syncp);
   + stats-tx_bytes += tx_bytes;
   + stats-tx_packets =+ tx_packets;
   + u64_stats_update_end(stats-tx_syncp);
}
  
static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
   --
   1.7.1
  --
  To unsubscribe from this list: send the line unsubscribe netdev in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()

[David Laight]

From: Michael S. Tsirkin
 On Wed, Oct 15, 2014 at 09:49:01AM +, David Laight wrote:
  From: Of Michael S. Tsirkin
   On Wed, Oct 15, 2014 at 03:25:27PM +0800, Jason Wang wrote:
Accumulate the sent packets and sent bytes in local variables and 
perform a
single u64_stats_update_begin/end() after.
   
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Jason Wang jasow...@redhat.com
  
   Not sure how much it's worth but since Eric suggested it ...
 
  Probably depends on the actual cost of u64_stats_update_begin/end
  against the likely extra saving of the tx_bytes and tx_packets
  values onto the stack across the call to dev_kfree_skb_any().
  (Which depends on the number of caller saved registers.)
 
 Yea, some benchmark results would be nice to see.

I there are likely to be multiple skb on the queue the fastest
code would probably do one 'virtqueue_get_all()' that returned
a linked list of buffers, then follow the list to get the stats,
and follow it again to free the skb.

David

   Acked-by: Michael S. Tsirkin m...@redhat.com
  
---
 drivers/net/virtio_net.c |   12 
 1 files changed, 8 insertions(+), 4 deletions(-)
   
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3d0ce44..a4d56b8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -833,17 +833,21 @@ static void free_old_xmit_skbs(struct send_queue 
*sq)
unsigned int len;
struct virtnet_info *vi = sq-vq-vdev-priv;
struct virtnet_stats *stats = this_cpu_ptr(vi-stats);
+   u64 tx_bytes = 0, tx_packets = 0;
 
  tx_packets need only be 'unsigned int'.
  The same is almost certainly true of tx_bytes.
 
  David
 
while ((skb = virtqueue_get_buf(sq-vq, len)) != NULL) {
pr_debug(Sent skb %p\n, skb);
   
-   u64_stats_update_begin(stats-tx_syncp);
-   stats-tx_bytes += skb-len;
-   stats-tx_packets++;
-   u64_stats_update_end(stats-tx_syncp);
+   tx_bytes += skb-len;
+   tx_packets++;
   
dev_kfree_skb_any(skb);
}
+
+   u64_stats_update_begin(stats-tx_syncp);
+   stats-tx_bytes += tx_bytes;
+   stats-tx_packets =+ tx_packets;
+   u64_stats_update_end(stats-tx_syncp);
 }
   
 static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
--
1.7.1
   --
   To unsubscribe from this list: send the line unsubscribe netdev in
   the body of a message to majord...@vger.kernel.org
   More majordomo info at  http://vger.kernel.org/majordomo-info.html
 


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()

[Michael S. Tsirkin]

On Wed, Oct 15, 2014 at 10:51:32AM +, David Laight wrote:
 From: Michael S. Tsirkin
  On Wed, Oct 15, 2014 at 09:49:01AM +, David Laight wrote:
   From: Of Michael S. Tsirkin
On Wed, Oct 15, 2014 at 03:25:27PM +0800, Jason Wang wrote:
 Accumulate the sent packets and sent bytes in local variables and 
 perform a
 single u64_stats_update_begin/end() after.

 Cc: Rusty Russell ru...@rustcorp.com.au
 Cc: Michael S. Tsirkin m...@redhat.com
 Signed-off-by: Jason Wang jasow...@redhat.com
   
Not sure how much it's worth but since Eric suggested it ...
  
   Probably depends on the actual cost of u64_stats_update_begin/end
   against the likely extra saving of the tx_bytes and tx_packets
   values onto the stack across the call to dev_kfree_skb_any().
   (Which depends on the number of caller saved registers.)
  
  Yea, some benchmark results would be nice to see.
 
 I there are likely to be multiple skb on the queue the fastest
 code would probably do one 'virtqueue_get_all()' that returned
 a linked list of buffers, then follow the list to get the stats,
 and follow it again to free the skb.
 
   David

Interesting. Each time we tried playing with linked list in the past,
it simply destroyed performance.
Maybe this case is different but I have my doubts.

Acked-by: Michael S. Tsirkin m...@redhat.com
   
 ---
  drivers/net/virtio_net.c |   12 
  1 files changed, 8 insertions(+), 4 deletions(-)

 diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
 index 3d0ce44..a4d56b8 100644
 --- a/drivers/net/virtio_net.c
 +++ b/drivers/net/virtio_net.c
 @@ -833,17 +833,21 @@ static void free_old_xmit_skbs(struct 
 send_queue *sq)
   unsigned int len;
   struct virtnet_info *vi = sq-vq-vdev-priv;
   struct virtnet_stats *stats = this_cpu_ptr(vi-stats);
 + u64 tx_bytes = 0, tx_packets = 0;
  
   tx_packets need only be 'unsigned int'.
   The same is almost certainly true of tx_bytes.
  
 David
  
   while ((skb = virtqueue_get_buf(sq-vq, len)) != NULL) {
   pr_debug(Sent skb %p\n, skb);

 - u64_stats_update_begin(stats-tx_syncp);
 - stats-tx_bytes += skb-len;
 - stats-tx_packets++;
 - u64_stats_update_end(stats-tx_syncp);
 + tx_bytes += skb-len;
 + tx_packets++;

   dev_kfree_skb_any(skb);
   }
 +
 + u64_stats_update_begin(stats-tx_syncp);
 + stats-tx_bytes += tx_bytes;
 + stats-tx_packets =+ tx_packets;
 + u64_stats_update_end(stats-tx_syncp);
  }

  static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 --
 1.7.1
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization