On 7/20/2016 2:13 PM, Yuanhan Liu wrote: > 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.
Great, I see. And thanks for detailed clarification. I'll send v2. > >> 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. Yes, I agree this is the BKM we should adopt and recommend users to use. Thanks, Jianfeng > >> By the way, if a fix patch should only involve those commits it will change? > IMO, logically, yes. > > --yliu