Re: Performance regression with virtio_net

Re: Performance regression with virtio_net

[Euan Kemp]

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

[Michael S. Tsirkin]

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. Tsirkin <mst@redhat.com>


diff --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

[Seth Forshee]

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. Tsirkin <mst@redhat.com>

This fixes the issue for me, downloads are faster and rx_length_errors
does not show any errors.

Tested-by: Seth Forshee <seth.forshee@canonical.com>

Thanks!

Seth

Re: Performance regression with virtio_net

[Euan Kemp]

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

[Michael S. Tsirkin]

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.

Performance regression with virtio_net

[Seth Forshee]

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

Re: Performance regression with virtio_net

[Michael S. Tsirkin]

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

Re: Performance regression with virtio_net

[Seth Forshee]

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

[Michael S. Tsirkin]

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

[Seth Forshee]

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