From: "Michael S. Tsirkin" <mst@redhat.com> To: Rusty Russell <rusty@rustcorp.com.au> Cc: linux-kernel@vger.kernel.org, Carsten Otte <cotte@de.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, linux390@de.ibm.com, Martin Schwidefsky <schwidefsky@de.ibm.com>, Heiko Carstens <heiko.carstens@de.ibm.com>, Shirley Ma <xma@us.ibm.com>, lguest@lists.ozlabs.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390@vger.kernel.org, kvm@vger.kernel.org, Krishna Kumar <krkumar2@in.ibm.com>, Tom Lendacky <tahm@linux.vnet.ibm.com>, steved@us.ibm.com, habanero@linux.vnet.ibm.com Subject: Re: [PATCHv2 10/14] virtio_net: limit xmit polling Date: Mon, 23 May 2011 14:19:00 +0300 [thread overview] Message-ID: <20110523111900.GB27212@redhat.com> (raw) In-Reply-To: <87boyutbjg.fsf@rustcorp.com.au> On Mon, May 23, 2011 at 11:37:15AM +0930, Rusty Russell wrote: > On Sun, 22 May 2011 15:10:08 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Sat, May 21, 2011 at 11:49:59AM +0930, Rusty Russell wrote: > > > On Fri, 20 May 2011 02:11:56 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > Current code might introduce a lot of latency variation > > > > if there are many pending bufs at the time we > > > > attempt to transmit a new one. This is bad for > > > > real-time applications and can't be good for TCP either. > > > > > > Do we have more than speculation to back that up, BTW? > > > > Need to dig this up: I thought we saw some reports of this on the list? > > I think so too, but a reference needs to be here too. > > It helps to have exact benchmarks on what's being tested, otherwise we > risk unexpected interaction with the other optimization patches. > > > > > struct sk_buff *skb; > > > > unsigned int len; > > > > - > > > > - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) { > > > > + bool c; > > > > + int n; > > > > + > > > > + /* We try to free up at least 2 skbs per one sent, so that we'll get > > > > + * all of the memory back if they are used fast enough. */ > > > > + for (n = 0; > > > > + ((c = virtqueue_get_capacity(vi->svq) < capacity) || n < 2) && > > > > + ((skb = virtqueue_get_buf(vi->svq, &len))); > > > > + ++n) { > > > > pr_debug("Sent skb %p\n", skb); > > > > vi->dev->stats.tx_bytes += skb->len; > > > > vi->dev->stats.tx_packets++; > > > > dev_kfree_skb_any(skb); > > > > } > > > > + return !c; > > > > > > This is for() abuse :) > > > > > > Why is the capacity check in there at all? Surely it's simpler to try > > > to free 2 skbs each time around? > > > > This is in case we can't use indirect: we want to free up > > enough buffers for the following add_buf to succeed. > > Sure, or we could just count the frags of the skb we're taking out, > which would be accurate for both cases and far more intuitive. > > ie. always try to free up twice as much as we're about to put in. > > Can we hit problems with OOM? Sure, but no worse than now... > The problem is that this "virtqueue_get_capacity()" returns the worst > case, not the normal case. So using it is deceptive. > Maybe just document this? I still believe capacity really needs to be decided at the virtqueue level, not in the driver. E.g. with indirect each skb uses a single entry: freeing 1 small skb is always enough to have space for a large one. I do understand how it seems a waste to leave direct space in the ring while we might in practice have space due to indirect. Didn't come up with a nice way to solve this yet - but 'no worse than now :)' > > I just wanted to localize the 2+MAX_SKB_FRAGS logic that tries to make > > sure we have enough space in the buffer. Another way to do > > that is with a define :). > > To do this properly, we should really be using the actual number of sg > elements needed, but we'd have to do most of xmit_skb beforehand so we > know how many. > > Cheers, > Rusty. Maybe I'm confused here. The problem isn't the failing add_buf for the given skb IIUC. What we are trying to do here is stop the queue *before xmit_skb fails*. We can't look at the number of fragments in the current skb - the next one can be much larger. That's why we check capacity after xmit_skb, not before it, right? -- MST
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> Cc: Krishna Kumar <krkumar2-xthvdsQ13ZrQT0dZR+AlfA@public.gmane.org>, Carsten Otte <cotte-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, lguest-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Shirley Ma <xma-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-s390-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, habanero-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org, Heiko Carstens <heiko.carstens-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, steved-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org, Christian Borntraeger <borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, Tom Lendacky <tahm-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>, Martin Schwidefsky <schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, linux390-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org Subject: Re: [PATCHv2 10/14] virtio_net: limit xmit polling Date: Mon, 23 May 2011 14:19:00 +0300 [thread overview] Message-ID: <20110523111900.GB27212@redhat.com> (raw) In-Reply-To: <87boyutbjg.fsf-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> On Mon, May 23, 2011 at 11:37:15AM +0930, Rusty Russell wrote: > On Sun, 22 May 2011 15:10:08 +0300, "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > On Sat, May 21, 2011 at 11:49:59AM +0930, Rusty Russell wrote: > > > On Fri, 20 May 2011 02:11:56 +0300, "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > > > Current code might introduce a lot of latency variation > > > > if there are many pending bufs at the time we > > > > attempt to transmit a new one. This is bad for > > > > real-time applications and can't be good for TCP either. > > > > > > Do we have more than speculation to back that up, BTW? > > > > Need to dig this up: I thought we saw some reports of this on the list? > > I think so too, but a reference needs to be here too. > > It helps to have exact benchmarks on what's being tested, otherwise we > risk unexpected interaction with the other optimization patches. > > > > > struct sk_buff *skb; > > > > unsigned int len; > > > > - > > > > - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) { > > > > + bool c; > > > > + int n; > > > > + > > > > + /* We try to free up at least 2 skbs per one sent, so that we'll get > > > > + * all of the memory back if they are used fast enough. */ > > > > + for (n = 0; > > > > + ((c = virtqueue_get_capacity(vi->svq) < capacity) || n < 2) && > > > > + ((skb = virtqueue_get_buf(vi->svq, &len))); > > > > + ++n) { > > > > pr_debug("Sent skb %p\n", skb); > > > > vi->dev->stats.tx_bytes += skb->len; > > > > vi->dev->stats.tx_packets++; > > > > dev_kfree_skb_any(skb); > > > > } > > > > + return !c; > > > > > > This is for() abuse :) > > > > > > Why is the capacity check in there at all? Surely it's simpler to try > > > to free 2 skbs each time around? > > > > This is in case we can't use indirect: we want to free up > > enough buffers for the following add_buf to succeed. > > Sure, or we could just count the frags of the skb we're taking out, > which would be accurate for both cases and far more intuitive. > > ie. always try to free up twice as much as we're about to put in. > > Can we hit problems with OOM? Sure, but no worse than now... > The problem is that this "virtqueue_get_capacity()" returns the worst > case, not the normal case. So using it is deceptive. > Maybe just document this? I still believe capacity really needs to be decided at the virtqueue level, not in the driver. E.g. with indirect each skb uses a single entry: freeing 1 small skb is always enough to have space for a large one. I do understand how it seems a waste to leave direct space in the ring while we might in practice have space due to indirect. Didn't come up with a nice way to solve this yet - but 'no worse than now :)' > > I just wanted to localize the 2+MAX_SKB_FRAGS logic that tries to make > > sure we have enough space in the buffer. Another way to do > > that is with a define :). > > To do this properly, we should really be using the actual number of sg > elements needed, but we'd have to do most of xmit_skb beforehand so we > know how many. > > Cheers, > Rusty. Maybe I'm confused here. The problem isn't the failing add_buf for the given skb IIUC. What we are trying to do here is stop the queue *before xmit_skb fails*. We can't look at the number of fragments in the current skb - the next one can be much larger. That's why we check capacity after xmit_skb, not before it, right? -- MST
next prev parent reply other threads:[~2011-05-23 11:19 UTC|newest] Thread overview: 132+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-19 23:10 [PATCHv2 00/14] virtio and vhost-net performance enhancements Michael S. Tsirkin 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-19 23:10 ` [PATCHv2 01/14] virtio: event index interface Michael S. Tsirkin 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-21 2:29 ` Rusty Russell 2011-05-21 2:29 ` Rusty Russell 2011-05-21 2:29 ` Rusty Russell 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-19 23:10 ` [PATCHv2 02/14] virtio ring: inline function to check for events Michael S. Tsirkin 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-21 2:29 ` Rusty Russell 2011-05-21 2:29 ` Rusty Russell 2011-05-21 2:29 ` Rusty Russell 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-19 23:10 ` [PATCHv2 03/14] virtio_ring: support event idx feature Michael S. Tsirkin 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-21 2:31 ` Rusty Russell 2011-05-21 2:31 ` Rusty Russell 2011-05-21 2:31 ` Rusty Russell 2011-05-19 23:10 ` [PATCHv2 04/14] vhost: support event index Michael S. Tsirkin 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-21 2:31 ` Rusty Russell 2011-05-21 2:31 ` Rusty Russell 2011-05-21 2:31 ` Rusty Russell 2011-05-19 23:10 ` Michael S. Tsirkin 2011-05-19 23:11 ` [PATCHv2 05/14] virtio_test: " Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-21 2:32 ` Rusty Russell 2011-05-21 2:32 ` Rusty Russell 2011-05-21 2:32 ` Rusty Russell 2011-05-19 23:11 ` [PATCHv2 06/14] virtio: add api for delayed callbacks Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-21 2:33 ` Rusty Russell 2011-05-21 2:33 ` Rusty Russell 2011-05-21 2:33 ` Rusty Russell 2011-05-19 23:11 ` [PATCHv2 07/14] virtio_net: delay TX callbacks Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:11 ` [PATCHv2 08/14] virtio_ring: Add capacity check API Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:11 ` [PATCHv2 09/14] virtio_net: fix TX capacity checks using new API Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-21 2:13 ` Rusty Russell 2011-05-21 2:13 ` Rusty Russell 2011-05-21 2:13 ` Rusty Russell 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:11 ` [PATCHv2 10/14] virtio_net: limit xmit polling Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-21 2:19 ` Rusty Russell 2011-05-21 2:19 ` Rusty Russell 2011-05-21 2:19 ` Rusty Russell 2011-05-22 12:10 ` Michael S. Tsirkin 2011-05-22 12:10 ` Michael S. Tsirkin 2011-05-23 2:07 ` Rusty Russell 2011-05-23 2:07 ` Rusty Russell 2011-05-23 11:19 ` Michael S. Tsirkin 2011-05-23 11:19 ` Michael S. Tsirkin [this message] 2011-05-23 11:19 ` Michael S. Tsirkin 2011-05-24 7:54 ` Krishna Kumar2 2011-05-24 7:54 ` Krishna Kumar2 2011-05-24 9:12 ` Michael S. Tsirkin 2011-05-24 9:12 ` Michael S. Tsirkin 2011-05-24 9:12 ` Michael S. Tsirkin 2011-05-24 9:27 ` Krishna Kumar2 2011-05-24 9:27 ` Krishna Kumar2 2011-05-24 11:29 ` Michael S. Tsirkin 2011-05-24 11:29 ` Michael S. Tsirkin 2011-05-24 12:50 ` Krishna Kumar2 2011-05-24 12:50 ` Krishna Kumar2 2011-05-24 13:52 ` Michael S. Tsirkin 2011-05-24 13:52 ` Michael S. Tsirkin 2011-05-24 13:52 ` Michael S. Tsirkin 2011-05-24 12:50 ` Krishna Kumar2 2011-05-24 11:29 ` Michael S. Tsirkin 2011-05-24 7:54 ` Krishna Kumar2 2011-05-25 1:28 ` Rusty Russell 2011-05-25 1:28 ` Rusty Russell 2011-05-25 5:50 ` Michael S. Tsirkin 2011-05-25 5:50 ` Michael S. Tsirkin 2011-05-25 5:50 ` Michael S. Tsirkin 2011-05-25 1:28 ` Rusty Russell 2011-05-25 1:35 ` Rusty Russell 2011-05-25 1:35 ` Rusty Russell 2011-05-25 1:35 ` Rusty Russell 2011-05-25 6:07 ` Michael S. Tsirkin 2011-05-25 6:07 ` Michael S. Tsirkin 2011-05-26 3:28 ` Rusty Russell 2011-05-26 3:28 ` Rusty Russell 2011-05-26 3:28 ` Rusty Russell 2011-05-28 20:02 ` Michael S. Tsirkin 2011-05-28 20:02 ` Michael S. Tsirkin 2011-05-30 6:27 ` Rusty Russell 2011-05-30 6:27 ` Rusty Russell 2011-05-30 6:27 ` Rusty Russell 2011-05-28 20:02 ` Michael S. Tsirkin 2011-05-25 6:07 ` Michael S. Tsirkin 2011-05-23 2:07 ` Rusty Russell 2011-05-22 12:10 ` Michael S. Tsirkin 2011-05-19 23:11 ` Michael S. Tsirkin 2011-05-19 23:12 ` [PATCHv2 11/14] virtio: don't delay avail index update Michael S. Tsirkin 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-21 2:26 ` Rusty Russell 2011-05-21 2:26 ` Rusty Russell 2011-05-21 2:26 ` Rusty Russell 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-19 23:12 ` [PATCHv2 12/14] virtio: 64 bit features Michael S. Tsirkin 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-19 23:12 ` [PATCHv2 13/14] virtio_test: update for " Michael S. Tsirkin 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-19 23:12 ` [PATCHv2 14/14] vhost: fix " Michael S. Tsirkin 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-19 23:12 ` Michael S. Tsirkin 2011-05-19 23:20 ` [PATCHv2 00/14] virtio and vhost-net performance enhancements David Miller 2011-05-19 23:20 ` David Miller 2011-05-20 7:51 ` Rusty Russell 2011-05-20 7:51 ` Rusty Russell 2011-05-20 7:51 ` Rusty Russell 2011-05-26 15:32 ` [PERF RESULTS] " Krishna Kumar2 2011-05-26 15:32 ` Krishna Kumar2 2011-05-26 15:42 ` Shirley Ma 2011-05-26 16:21 ` Krishna Kumar2 2011-05-26 16:21 ` Krishna Kumar2 [not found] ` <OFF9D0E604.B865A006-ON6525789C.00597010-6525789C.0059987A@LocalDomain> 2011-05-26 16:29 ` Krishna Kumar2 2011-05-26 16:29 ` Krishna Kumar2 2011-05-26 16:29 ` Krishna Kumar2 2011-05-26 15:32 ` Krishna Kumar2
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110523111900.GB27212@redhat.com \ --to=mst@redhat.com \ --cc=borntraeger@de.ibm.com \ --cc=cotte@de.ibm.com \ --cc=habanero@linux.vnet.ibm.com \ --cc=heiko.carstens@de.ibm.com \ --cc=krkumar2@in.ibm.com \ --cc=kvm@vger.kernel.org \ --cc=lguest@lists.ozlabs.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=linux390@de.ibm.com \ --cc=netdev@vger.kernel.org \ --cc=rusty@rustcorp.com.au \ --cc=schwidefsky@de.ibm.com \ --cc=steved@us.ibm.com \ --cc=tahm@linux.vnet.ibm.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=xma@us.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.