Re: Performance regression with virtio_net
On Mon, Jul 31, 2017 at 11:38:00AM -0700, Euan Kemp wrote: > On Mon, Jul 31, 2017 at 04:26:08PM +0300, Michael S. Tsirkin wrote: > > I kept this around unchanged from > > ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal > > reason not to account for that space: not enough bits to do it. No > > longer true so let's account for length exactly. I'll send a proper > > patch after a bit of testing, would appreciate reports reports of > > whether this helps very much. > > I can confirm your patch fixes the issue for me. > > - Euan Patch posted, thanks everyone.
Re: Performance regression with virtio_net
On Mon, Jul 31, 2017 at 04:26:08PM +0300, Michael S. Tsirkin wrote: > I kept this around unchanged from > ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal > reason not to account for that space: not enough bits to do it. No > longer true so let's account for length exactly. I'll send a proper > patch after a bit of testing, would appreciate reports reports of > whether this helps very much. I can confirm your patch fixes the issue for me. - Euan
Re: Performance regression with virtio_net
On Mon, Jul 31, 2017 at 04:26:08PM +0300, Michael S. Tsirkin wrote: > On Sun, Jul 30, 2017 at 03:25:52PM -0700, Euan Kemp wrote: > > I've also observed this performance regression. > > > > The minimal fix for me is removing the two > > > if (unlikely(len > (unsigned long)ctx)) > > checks added in 680557c. > > > > After digging a little more, the reason that check can fail appears to > > be that add_recvbuf_mergeable sometimes includes a hole at the end, > > which is included in len but not ctx. > > > > I'd send a patch removing those conditions, but I'm not certain > > whether "truesize" in receive_mergeable should also be changed back to > > be the max of len/ctx, or should remain as-is. > > > > - Euan > > Thanks a lot for looking into it! > > I kept this around unchanged from > ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal > reason not to account for that space: not enough bits to do it. No > longer true so let's account for length exactly. I'll send a proper > patch after a bit of testing, would appreciate reports reports of > whether this helps very much. > > Signed-off-by: Michael S. TsirkinThis fixes the issue for me, downloads are faster and rx_length_errors does not show any errors. Tested-by: Seth Forshee Thanks! Seth
Re: Performance regression with virtio_net
On Sun, Jul 30, 2017 at 03:25:52PM -0700, Euan Kemp wrote: > I've also observed this performance regression. > > The minimal fix for me is removing the two > > if (unlikely(len > (unsigned long)ctx)) > checks added in 680557c. > > After digging a little more, the reason that check can fail appears to > be that add_recvbuf_mergeable sometimes includes a hole at the end, > which is included in len but not ctx. > > I'd send a patch removing those conditions, but I'm not certain > whether "truesize" in receive_mergeable should also be changed back to > be the max of len/ctx, or should remain as-is. > > - Euan Thanks a lot for looking into it! I kept this around unchanged from ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal reason not to account for that space: not enough bits to do it. No longer true so let's account for length exactly. I'll send a proper patch after a bit of testing, would appreciate reports reports of whether this helps very much. Signed-off-by: Michael S. Tsirkindiff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index f41ab0e..782c33f 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -889,7 +889,6 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi, buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset; buf += headroom; /* advance address leaving hole at front of pkt */ - ctx = (void *)(unsigned long)len; get_page(alloc_frag->page); alloc_frag->offset += len + headroom; hole = alloc_frag->size - alloc_frag->offset; @@ -904,6 +903,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi, } sg_init_one(rq->sg, buf, len); + ctx = (void *)(unsigned long)len; err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp); if (err < 0) put_page(virt_to_head_page(buf)); -- MST
Re: Performance regression with virtio_net
I've also observed this performance regression. The minimal fix for me is removing the two > if (unlikely(len > (unsigned long)ctx)) checks added in 680557c. After digging a little more, the reason that check can fail appears to be that add_recvbuf_mergeable sometimes includes a hole at the end, which is included in len but not ctx. I'd send a patch removing those conditions, but I'm not certain whether "truesize" in receive_mergeable should also be changed back to be the max of len/ctx, or should remain as-is. - Euan
Re: Performance regression with virtio_net
On Fri, Jul 28, 2017 at 12:30:54AM +0300, Michael S. Tsirkin wrote: > On Thu, Jul 27, 2017 at 04:14:30PM -0500, Seth Forshee wrote: > > On Thu, Jul 27, 2017 at 11:38:52PM +0300, Michael S. Tsirkin wrote: > > > On Thu, Jul 27, 2017 at 12:09:42PM -0500, Seth Forshee wrote: > > > > I'm seeing a performance regression with virtio_net that looks to have > > > > started in 4.12-rc1. I only see it in one context though, downloading > > > > snap packages from the Ubuntu snap store. For example: > > > > > > > > > > > > https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap > > > > > > > > which redirects to Internap's CDN. Normally this downloads in a few > > > > seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate > > > > of ~150 KB/s. Everything else I've tried downloads as normal speeds. > > > > > > So just wget that URL should be enough? > > > > Yes. Note that sometimes it starts out faster then slows down. > > > > I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer > > > > handling". If I revert this on top of 4.13-rc2 (along with other changes > > > > needed to successfully revert it) speeds return to normal. > > > > > > > > Thanks, > > > > Seth > > > > > > > > > Interesting. A more likely suspect would be > > > e377fcc8486d40867c6c217077ad0fa40977e060 - could you please try > > > reverting that one instead? > > > > I tried it, and I still get slow download speeds. I did test at > > 680557cf79f82623e2c4fd42733077d60a843513 during the bisect so I'm > > reasonably confident that this is the one where things went bad. > > > Also, could you please look at mergeable_rx_buffer_size in sysfs with > > > and without the change? > > > > In all cases (stock 4.13-rc2, 680557cf79f8 reverted, and e377fcc8486d > > reverted) mergeable_rx_buffer_size was 1536. > > > > Thanks, > > Seth > > Do you see any error counters incrementing after it slows down? I see rx_dropped and rx_length_errors increasing in lockstep once it slows down. Thanks, Seth
Re: Performance regression with virtio_net
On Thu, Jul 27, 2017 at 04:14:30PM -0500, Seth Forshee wrote: > On Thu, Jul 27, 2017 at 11:38:52PM +0300, Michael S. Tsirkin wrote: > > On Thu, Jul 27, 2017 at 12:09:42PM -0500, Seth Forshee wrote: > > > I'm seeing a performance regression with virtio_net that looks to have > > > started in 4.12-rc1. I only see it in one context though, downloading > > > snap packages from the Ubuntu snap store. For example: > > > > > > > > > https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap > > > > > > which redirects to Internap's CDN. Normally this downloads in a few > > > seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate > > > of ~150 KB/s. Everything else I've tried downloads as normal speeds. > > > > So just wget that URL should be enough? > > Yes. Note that sometimes it starts out faster then slows down. > > > I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer > > > handling". If I revert this on top of 4.13-rc2 (along with other changes > > > needed to successfully revert it) speeds return to normal. > > > > > > Thanks, > > > Seth > > > > > > Interesting. A more likely suspect would be > > e377fcc8486d40867c6c217077ad0fa40977e060 - could you please try > > reverting that one instead? > > I tried it, and I still get slow download speeds. I did test at > 680557cf79f82623e2c4fd42733077d60a843513 during the bisect so I'm > reasonably confident that this is the one where things went bad. > > Also, could you please look at mergeable_rx_buffer_size in sysfs with > > and without the change? > > In all cases (stock 4.13-rc2, 680557cf79f8 reverted, and e377fcc8486d > reverted) mergeable_rx_buffer_size was 1536. > > Thanks, > Seth Do you see any error counters incrementing after it slows down? -- MST
Re: Performance regression with virtio_net
On Thu, Jul 27, 2017 at 11:38:52PM +0300, Michael S. Tsirkin wrote: > On Thu, Jul 27, 2017 at 12:09:42PM -0500, Seth Forshee wrote: > > I'm seeing a performance regression with virtio_net that looks to have > > started in 4.12-rc1. I only see it in one context though, downloading > > snap packages from the Ubuntu snap store. For example: > > > > > > https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap > > > > which redirects to Internap's CDN. Normally this downloads in a few > > seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate > > of ~150 KB/s. Everything else I've tried downloads as normal speeds. > > So just wget that URL should be enough? Yes. Note that sometimes it starts out faster then slows down. > > I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer > > handling". If I revert this on top of 4.13-rc2 (along with other changes > > needed to successfully revert it) speeds return to normal. > > > > Thanks, > > Seth > > > Interesting. A more likely suspect would be > e377fcc8486d40867c6c217077ad0fa40977e060 - could you please try > reverting that one instead? I tried it, and I still get slow download speeds. I did test at 680557cf79f82623e2c4fd42733077d60a843513 during the bisect so I'm reasonably confident that this is the one where things went bad. > Also, could you please look at mergeable_rx_buffer_size in sysfs with > and without the change? In all cases (stock 4.13-rc2, 680557cf79f8 reverted, and e377fcc8486d reverted) mergeable_rx_buffer_size was 1536. Thanks, Seth
Re: Performance regression with virtio_net
On Thu, Jul 27, 2017 at 12:09:42PM -0500, Seth Forshee wrote: > I'm seeing a performance regression with virtio_net that looks to have > started in 4.12-rc1. I only see it in one context though, downloading > snap packages from the Ubuntu snap store. For example: > > > https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap > > which redirects to Internap's CDN. Normally this downloads in a few > seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate > of ~150 KB/s. Everything else I've tried downloads as normal speeds. So just wget that URL should be enough? > I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer > handling". If I revert this on top of 4.13-rc2 (along with other changes > needed to successfully revert it) speeds return to normal. > > Thanks, > Seth Interesting. A more likely suspect would be e377fcc8486d40867c6c217077ad0fa40977e060 - could you please try reverting that one instead? Also, could you please look at mergeable_rx_buffer_size in sysfs with and without the change? -- MST
Performance regression with virtio_net
I'm seeing a performance regression with virtio_net that looks to have started in 4.12-rc1. I only see it in one context though, downloading snap packages from the Ubuntu snap store. For example: https://api.snapcraft.io/api/v1/snaps/download/b8X2psL1ryVrPt5WEmpYiqfr5emixTd7_1797.snap which redirects to Internap's CDN. Normally this downloads in a few seconds at ~10 MB/s, but with 4.12 and 4.13 it takes minutes with a rate of ~150 KB/s. Everything else I've tried downloads as normal speeds. I bisected this to 680557cf79f8 "virtio_net: rework mergeable buffer handling". If I revert this on top of 4.13-rc2 (along with other changes needed to successfully revert it) speeds return to normal. Thanks, Seth