Re: [dpdk-dev] [PATCH] examples/vhost: fix perf regression

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Wed, Jul 20, 2016 at 01:50:34PM +0800, Tan, Jianfeng wrote:
> 
> 
> On 7/20/2016 12:38 PM, Yuanhan Liu wrote:
> >On Tue, Jul 19, 2016 at 01:53:11PM +0000, Jianfeng Tan wrote:
> >>We find significant perfermance drop introduced by below commit,
> >>when vhost example is started with --mergeable 0 and inside vm,
> >>kernel virtio-net driver is used to do ip based forwarding.
> >>
> >>The root cause is that below commit adds support for
> >>VIRTIO_NET_F_GUEST_TSO4 and VIRTIO_NET_F_GUEST_TSO6, and when
> >>mergeable is disabled, it triggers big_packets path of virtio-net
> >>driver. In this path, virtio driver uses 19 desc with 18 4K-sized
> >>pages to receive each packet, so that it can receive a big packet
> >>with size of 64K. But QEMU only creates 256 desc entries for each
> >>vq, which results in that only 13 packets can be received. VM
> >>kernel can quickly handle those packets and go to sleep (HLT).
> >>
> >>As QEMU has no option to set the desc entries of a vq, so here,
> >>we disable VIRTIO_NET_F_GUEST_TSO4 and VIRTIO_NET_F_GUEST_TSO6
> >>with VIRTIO_NET_F_HOST_TSO4 and VIRTIO_NET_F_HOST_TSO6 when we
> >>disable tso of vhost example, to avoid VM kernel virtio driver
> >>go into big_packets path.
> >>
> >>Fixes: 859b480d5afd ("vhost: add guest offload setting")
> >And here you are patching vhost example to try to fix an "issue"
> >in vhost lib, this is __logically__ wrong.
> >
> >	--yliu
> 
> This is not an issue from vhost lib's perspective, vhost lib should provide
> all features it supports by default.

Bingo.., that's why "Fixes: 859b480d5afd ... " is wrong to me.
  
> Applications can enable/disable
> features according to their own requirements.

Yes, application can, but application normally doesn't do that. And
as stated in my early reply, the qemu is the place you should go for
all those options enabling/disabling, but not vhost (not vhost-example).

I think it's sometimes more handy if we can do that by introducing
some vhost-example options, and I guess that's why those options are
given.

In another word, there is nothing wrong about the commit 859b480d5afd,
if you want to "fix" anything here, following commit is something
we need fix:

    Fixes: 9fd72e3cbd29 ("examples/vhost: add virtio offload")

Because that commit just partially disables some TSO related features,
letting the virtio net driver goes to the slow path.

> And the vhost example after
> this commit just triggers a slow path of virtio driver. So this fix just
> makes sure vhost example does not go into the slow path by default.

I have made a statement in the first time, that I am not object to
have this patch at all.

Meanwhile, the right "fix" is you need disable all TSO related features
from QEMU, in such way, we should see no such issue from all vhost
application, but not only this one, the one we used mostly internally.

As you can see, it's more about the usage.

> By the way, if a fix patch should only involve those commits it will change?

IMO, logically, yes.

	--yliu