David Laight
2014-Oct-15 09:49 UTC
[RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()
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 <rusty at rustcorp.com.au> > > Cc: Michael S. Tsirkin <mst at redhat.com> > > Signed-off-by: Jason Wang <jasowang at 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.)> Acked-by: Michael S. Tsirkin <mst at 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 majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin
2014-Oct-15 10:48 UTC
[RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()
On Wed, Oct 15, 2014 at 09:49:01AM +0000, 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 <rusty at rustcorp.com.au> > > > Cc: Michael S. Tsirkin <mst at redhat.com> > > > Signed-off-by: Jason Wang <jasowang at 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 <mst at 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 majordomo at vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html >
David Laight
2014-Oct-15 10:51 UTC
[RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()
From: Michael S. Tsirkin> On Wed, Oct 15, 2014 at 09:49:01AM +0000, 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 <rusty at rustcorp.com.au> > > > > Cc: Michael S. Tsirkin <mst at redhat.com> > > > > Signed-off-by: Jason Wang <jasowang at 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 <mst at 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 majordomo at vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Possibly Parallel Threads
- [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()
- [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()
- [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()
- [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()
- [RFC PATCH net-next 3/6] virtio-net: small optimization on free_old_xmit_skbs()