From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 67F4AC43441 for ; Fri, 9 Nov 2018 10:04:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3679820883 for ; Fri, 9 Nov 2018 10:04:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3679820883 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728001AbeKITop (ORCPT ); Fri, 9 Nov 2018 14:44:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53092 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727552AbeKITop (ORCPT ); Fri, 9 Nov 2018 14:44:45 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 76DD1C05E144; Fri, 9 Nov 2018 10:04:53 +0000 (UTC) Received: from [10.72.12.67] (ovpn-12-67.pek2.redhat.com [10.72.12.67]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B05195D6B5; Fri, 9 Nov 2018 10:04:44 +0000 (UTC) Subject: Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support To: "Michael S. Tsirkin" Cc: Tiwei Bie , virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, virtio-dev@lists.oasis-open.org, wexu@redhat.com, jfreimann@redhat.com References: <20180711022711.7090-1-tiwei.bie@intel.com> <20180711022711.7090-4-tiwei.bie@intel.com> <20181107123933-mutt-send-email-mst@kernel.org> <20181108013759.GA20591@debian> <2d46a41e-bc00-276a-e19a-105c9dffc75a@redhat.com> <20181108091337-mutt-send-email-mst@kernel.org> <21d6dbd9-8f78-6939-0e80-27b470aeb00a@redhat.com> <20181108225555-mutt-send-email-mst@kernel.org> From: Jason Wang Message-ID: Date: Fri, 9 Nov 2018 18:04:42 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20181108225555-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 09 Nov 2018 10:04:53 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2018/11/9 上午11:58, Michael S. Tsirkin wrote: > On Fri, Nov 09, 2018 at 10:25:28AM +0800, Jason Wang wrote: >> On 2018/11/8 下午10:14, Michael S. Tsirkin wrote: >>> On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote: >>>> On 2018/11/8 上午9:38, Tiwei Bie wrote: >>>>>>> + >>>>>>> + if (vq->vq.num_free < descs_used) { >>>>>>> + pr_debug("Can't add buf len %i - avail = %i\n", >>>>>>> + descs_used, vq->vq.num_free); >>>>>>> + /* FIXME: for historical reasons, we force a notify here if >>>>>>> + * there are outgoing parts to the buffer. Presumably the >>>>>>> + * host should service the ring ASAP. */ >>>>>> I don't think we have a reason to do this for packed ring. >>>>>> No historical baggage there, right? >>>>> Based on the original commit log, it seems that the notify here >>>>> is just an "optimization". But I don't quite understand what does >>>>> the "the heuristics which KVM uses" refer to. If it's safe to drop >>>>> this in packed ring, I'd like to do it. >>>> According to the commit log, it seems like a workaround of lguest networking >>>> backend. I agree to drop it, we should not have such burden. >>>> >>>> But we should notice that, with this removed, the compare between packed vs >>>> split is kind of unfair. >>> I don't think this ever triggers to be frank. When would it? >> >> I think it can happen e.g in the path of XDP transmission in >> __virtnet_xdp_xmit_one(): >> >> >>         err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC); >>         if (unlikely(err)) >>                 return -ENOSPC; /* Caller handle free/refcnt */ >> > I see. We used to do it for regular xmit but stopped > doing it. Is it fine for xdp then? There's no traffic control in XDP, so it was the only thing we can do. > >>>> Consider the removal of lguest support recently, >>>> maybe we can drop this for split ring as well? >>>> >>>> Thanks >>> If it's helpful, then for sure we can drop it for virtio 1. >>> Can you see any perf differences at all? With which device? >> >> I don't test but consider the case of XDP_TX in guest plus vhost_net in >> host. Since vhost_net is half duplex, it's pretty easier to trigger this >> condition. >> >> Thanks > Sounds reasonable. Worth testing before we change things though. Let me test and submit a patch. Thanks > >>>>> commit 44653eae1407f79dff6f52fcf594ae84cb165ec4 >>>>> Author: Rusty Russell >>>>> Date: Fri Jul 25 12:06:04 2008 -0500 >>>>> >>>>> virtio: don't always force a notification when ring is full >>>>> We force notification when the ring is full, even if the host has >>>>> indicated it doesn't want to know. This seemed like a good idea at >>>>> the time: if we fill the transmit ring, we should tell the host >>>>> immediately. >>>>> Unfortunately this logic also applies to the receiving ring, which is >>>>> refilled constantly. We should introduce real notification thesholds >>>>> to replace this logic. Meanwhile, removing the logic altogether breaks >>>>> the heuristics which KVM uses, so we use a hack: only notify if there are >>>>> outgoing parts of the new buffer. >>>>> Here are the number of exits with lguest's crappy network implementation: >>>>> Before: >>>>> network xmit 7859051 recv 236420 >>>>> After: >>>>> network xmit 7858610 recv 118136 >>>>> Signed-off-by: Rusty Russell >>>>> >>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >>>>> index 72bf8bc09014..21d9a62767af 100644 >>>>> --- a/drivers/virtio/virtio_ring.c >>>>> +++ b/drivers/virtio/virtio_ring.c >>>>> @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq, >>>>> if (vq->num_free < out + in) { >>>>> pr_debug("Can't add buf len %i - avail = %i\n", >>>>> out + in, vq->num_free); >>>>> - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */ >>>>> - vq->notify(&vq->vq); >>>>> + /* FIXME: for historical reasons, we force a notify here if >>>>> + * there are outgoing parts to the buffer. Presumably the >>>>> + * host should service the ring ASAP. */ >>>>> + if (out) >>>>> + vq->notify(&vq->vq); >>>>> END_USE(vq); >>>>> return -ENOSPC; >>>>> } >>>>> >>>>> From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support Date: Fri, 9 Nov 2018 18:04:42 +0800 Message-ID: References: <20180711022711.7090-1-tiwei.bie@intel.com> <20180711022711.7090-4-tiwei.bie@intel.com> <20181107123933-mutt-send-email-mst@kernel.org> <20181108013759.GA20591@debian> <2d46a41e-bc00-276a-e19a-105c9dffc75a@redhat.com> <20181108091337-mutt-send-email-mst@kernel.org> <21d6dbd9-8f78-6939-0e80-27b470aeb00a@redhat.com> <20181108225555-mutt-send-email-mst@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Cc: virtio-dev@lists.oasis-open.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, wexu@redhat.com To: "Michael S. Tsirkin" Return-path: In-Reply-To: <20181108225555-mutt-send-email-mst@kernel.org> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: netdev.vger.kernel.org Ck9uIDIwMTgvMTEvOSDkuIrljYgxMTo1OCwgTWljaGFlbCBTLiBUc2lya2luIHdyb3RlOgo+IE9u IEZyaSwgTm92IDA5LCAyMDE4IGF0IDEwOjI1OjI4QU0gKzA4MDAsIEphc29uIFdhbmcgd3JvdGU6 Cj4+IE9uIDIwMTgvMTEvOCDkuIvljYgxMDoxNCwgTWljaGFlbCBTLiBUc2lya2luIHdyb3RlOgo+ Pj4gT24gVGh1LCBOb3YgMDgsIDIwMTggYXQgMDQ6MTg6MjVQTSArMDgwMCwgSmFzb24gV2FuZyB3 cm90ZToKPj4+PiBPbiAyMDE4LzExLzgg5LiK5Y2IOTozOCwgVGl3ZWkgQmllIHdyb3RlOgo+Pj4+ Pj4+ICsKPj4+Pj4+PiArCWlmICh2cS0+dnEubnVtX2ZyZWUgPCBkZXNjc191c2VkKSB7Cj4+Pj4+ Pj4gKwkJcHJfZGVidWcoIkNhbid0IGFkZCBidWYgbGVuICVpIC0gYXZhaWwgPSAlaVxuIiwKPj4+ Pj4+PiArCQkJIGRlc2NzX3VzZWQsIHZxLT52cS5udW1fZnJlZSk7Cj4+Pj4+Pj4gKwkJLyogRklY TUU6IGZvciBoaXN0b3JpY2FsIHJlYXNvbnMsIHdlIGZvcmNlIGEgbm90aWZ5IGhlcmUgaWYKPj4+ Pj4+PiArCQkgKiB0aGVyZSBhcmUgb3V0Z29pbmcgcGFydHMgdG8gdGhlIGJ1ZmZlci4gIFByZXN1 bWFibHkgdGhlCj4+Pj4+Pj4gKwkJICogaG9zdCBzaG91bGQgc2VydmljZSB0aGUgcmluZyBBU0FQ LiAqLwo+Pj4+Pj4gSSBkb24ndCB0aGluayB3ZSBoYXZlIGEgcmVhc29uIHRvIGRvIHRoaXMgZm9y IHBhY2tlZCByaW5nLgo+Pj4+Pj4gTm8gaGlzdG9yaWNhbCBiYWdnYWdlIHRoZXJlLCByaWdodD8K Pj4+Pj4gQmFzZWQgb24gdGhlIG9yaWdpbmFsIGNvbW1pdCBsb2csIGl0IHNlZW1zIHRoYXQgdGhl IG5vdGlmeSBoZXJlCj4+Pj4+IGlzIGp1c3QgYW4gIm9wdGltaXphdGlvbiIuIEJ1dCBJIGRvbid0 IHF1aXRlIHVuZGVyc3RhbmQgd2hhdCBkb2VzCj4+Pj4+IHRoZSAidGhlIGhldXJpc3RpY3Mgd2hp Y2ggS1ZNIHVzZXMiIHJlZmVyIHRvLiBJZiBpdCdzIHNhZmUgdG8gZHJvcAo+Pj4+PiB0aGlzIGlu IHBhY2tlZCByaW5nLCBJJ2QgbGlrZSB0byBkbyBpdC4KPj4+PiBBY2NvcmRpbmcgdG8gdGhlIGNv bW1pdCBsb2csIGl0IHNlZW1zIGxpa2UgYSB3b3JrYXJvdW5kIG9mIGxndWVzdCBuZXR3b3JraW5n Cj4+Pj4gYmFja2VuZC4gSSBhZ3JlZSB0byBkcm9wIGl0LCB3ZSBzaG91bGQgbm90IGhhdmUgc3Vj aCBidXJkZW4uCj4+Pj4KPj4+PiBCdXQgd2Ugc2hvdWxkIG5vdGljZSB0aGF0LCB3aXRoIHRoaXMg cmVtb3ZlZCwgdGhlIGNvbXBhcmUgYmV0d2VlbiBwYWNrZWQgdnMKPj4+PiBzcGxpdCBpcyBraW5k IG9mIHVuZmFpci4KPj4+IEkgZG9uJ3QgdGhpbmsgdGhpcyBldmVyIHRyaWdnZXJzIHRvIGJlIGZy YW5rLiBXaGVuIHdvdWxkIGl0Pwo+Pgo+PiBJIHRoaW5rIGl0IGNhbiBoYXBwZW4gZS5nIGluIHRo ZSBwYXRoIG9mIFhEUCB0cmFuc21pc3Npb24gaW4KPj4gX192aXJ0bmV0X3hkcF94bWl0X29uZSgp Ogo+Pgo+Pgo+PiAgwqDCoMKgwqDCoMKgwqAgZXJyID0gdmlydHF1ZXVlX2FkZF9vdXRidWYoc3Et PnZxLCBzcS0+c2csIDEsIHhkcGYsIEdGUF9BVE9NSUMpOwo+PiAgwqDCoMKgwqDCoMKgwqAgaWYg KHVubGlrZWx5KGVycikpCj4+ICDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgcmV0dXJu IC1FTk9TUEM7IC8qIENhbGxlciBoYW5kbGUgZnJlZS9yZWZjbnQgKi8KPj4KPiBJIHNlZS4gV2Ug dXNlZCB0byBkbyBpdCBmb3IgcmVndWxhciB4bWl0IGJ1dCBzdG9wcGVkCj4gZG9pbmcgaXQuIElz IGl0IGZpbmUgZm9yIHhkcCB0aGVuPwoKClRoZXJlJ3Mgbm8gdHJhZmZpYyBjb250cm9sIGluIFhE UCwgc28gaXQgd2FzIHRoZSBvbmx5IHRoaW5nIHdlIGNhbiBkby4KCgo+Cj4+Pj4gQ29uc2lkZXIg dGhlIHJlbW92YWwgb2YgbGd1ZXN0IHN1cHBvcnQgcmVjZW50bHksCj4+Pj4gbWF5YmUgd2UgY2Fu IGRyb3AgdGhpcyBmb3Igc3BsaXQgcmluZyBhcyB3ZWxsPwo+Pj4+Cj4+Pj4gVGhhbmtzCj4+PiBJ ZiBpdCdzIGhlbHBmdWwsIHRoZW4gZm9yIHN1cmUgd2UgY2FuIGRyb3AgaXQgZm9yIHZpcnRpbyAx Lgo+Pj4gQ2FuIHlvdSBzZWUgYW55IHBlcmYgZGlmZmVyZW5jZXMgYXQgYWxsPyBXaXRoIHdoaWNo IGRldmljZT8KPj4KPj4gSSBkb24ndCB0ZXN0IGJ1dCBjb25zaWRlciB0aGUgY2FzZSBvZiBYRFBf VFggaW4gZ3Vlc3QgcGx1cyB2aG9zdF9uZXQgaW4KPj4gaG9zdC4gU2luY2Ugdmhvc3RfbmV0IGlz IGhhbGYgZHVwbGV4LCBpdCdzIHByZXR0eSBlYXNpZXIgdG8gdHJpZ2dlciB0aGlzCj4+IGNvbmRp dGlvbi4KPj4KPj4gVGhhbmtzCj4gU291bmRzIHJlYXNvbmFibGUuIFdvcnRoIHRlc3RpbmcgYmVm b3JlIHdlIGNoYW5nZSB0aGluZ3MgdGhvdWdoLgoKCkxldCBtZSB0ZXN0IGFuZCBzdWJtaXQgYSBw YXRjaC4KClRoYW5rcwoKCj4KPj4+Pj4gY29tbWl0IDQ0NjUzZWFlMTQwN2Y3OWRmZjZmNTJmY2Y1 OTRhZTg0Y2IxNjVlYzQKPj4+Pj4gQXV0aG9yOiBSdXN0eSBSdXNzZWxsPHJ1c3R5QHJ1c3Rjb3Jw LmNvbS5hdT4KPj4+Pj4gRGF0ZTogICBGcmkgSnVsIDI1IDEyOjA2OjA0IDIwMDggLTA1MDAKPj4+ Pj4KPj4+Pj4gICAgICAgIHZpcnRpbzogZG9uJ3QgYWx3YXlzIGZvcmNlIGEgbm90aWZpY2F0aW9u IHdoZW4gcmluZyBpcyBmdWxsCj4+Pj4+ICAgICAgICBXZSBmb3JjZSBub3RpZmljYXRpb24gd2hl biB0aGUgcmluZyBpcyBmdWxsLCBldmVuIGlmIHRoZSBob3N0IGhhcwo+Pj4+PiAgICAgICAgaW5k aWNhdGVkIGl0IGRvZXNuJ3Qgd2FudCB0byBrbm93LiAgVGhpcyBzZWVtZWQgbGlrZSBhIGdvb2Qg aWRlYSBhdAo+Pj4+PiAgICAgICAgdGhlIHRpbWU6IGlmIHdlIGZpbGwgdGhlIHRyYW5zbWl0IHJp bmcsIHdlIHNob3VsZCB0ZWxsIHRoZSBob3N0Cj4+Pj4+ICAgICAgICBpbW1lZGlhdGVseS4KPj4+ Pj4gICAgICAgIFVuZm9ydHVuYXRlbHkgdGhpcyBsb2dpYyBhbHNvIGFwcGxpZXMgdG8gdGhlIHJl Y2VpdmluZyByaW5nLCB3aGljaCBpcwo+Pj4+PiAgICAgICAgcmVmaWxsZWQgY29uc3RhbnRseS4g IFdlIHNob3VsZCBpbnRyb2R1Y2UgcmVhbCBub3RpZmljYXRpb24gdGhlc2hvbGRzCj4+Pj4+ICAg ICAgICB0byByZXBsYWNlIHRoaXMgbG9naWMuICBNZWFud2hpbGUsIHJlbW92aW5nIHRoZSBsb2dp YyBhbHRvZ2V0aGVyIGJyZWFrcwo+Pj4+PiAgICAgICAgdGhlIGhldXJpc3RpY3Mgd2hpY2ggS1ZN IHVzZXMsIHNvIHdlIHVzZSBhIGhhY2s6IG9ubHkgbm90aWZ5IGlmIHRoZXJlIGFyZQo+Pj4+PiAg ICAgICAgb3V0Z29pbmcgcGFydHMgb2YgdGhlIG5ldyBidWZmZXIuCj4+Pj4+ICAgICAgICBIZXJl IGFyZSB0aGUgbnVtYmVyIG9mIGV4aXRzIHdpdGggbGd1ZXN0J3MgY3JhcHB5IG5ldHdvcmsgaW1w bGVtZW50YXRpb246Cj4+Pj4+ICAgICAgICBCZWZvcmU6Cj4+Pj4+ICAgICAgICAgICAgICAgIG5l dHdvcmsgeG1pdCA3ODU5MDUxIHJlY3YgMjM2NDIwCj4+Pj4+ICAgICAgICBBZnRlcjoKPj4+Pj4g ICAgICAgICAgICAgICAgbmV0d29yayB4bWl0IDc4NTg2MTAgcmVjdiAxMTgxMzYKPj4+Pj4gICAg ICAgIFNpZ25lZC1vZmYtYnk6IFJ1c3R5IFJ1c3NlbGw8cnVzdHlAcnVzdGNvcnAuY29tLmF1Pgo+ Pj4+Pgo+Pj4+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy92aXJ0aW8vdmlydGlvX3JpbmcuYyBiL2Ry aXZlcnMvdmlydGlvL3ZpcnRpb19yaW5nLmMKPj4+Pj4gaW5kZXggNzJiZjhiYzA5MDE0Li4yMWQ5 YTYyNzY3YWYgMTAwNjQ0Cj4+Pj4+IC0tLSBhL2RyaXZlcnMvdmlydGlvL3ZpcnRpb19yaW5nLmMK Pj4+Pj4gKysrIGIvZHJpdmVycy92aXJ0aW8vdmlydGlvX3JpbmcuYwo+Pj4+PiBAQCAtODcsOCAr ODcsMTEgQEAgc3RhdGljIGludCB2cmluZ19hZGRfYnVmKHN0cnVjdCB2aXJ0cXVldWUgKl92cSwK Pj4+Pj4gICAgIAlpZiAodnEtPm51bV9mcmVlIDwgb3V0ICsgaW4pIHsKPj4+Pj4gICAgIAkJcHJf ZGVidWcoIkNhbid0IGFkZCBidWYgbGVuICVpIC0gYXZhaWwgPSAlaVxuIiwKPj4+Pj4gICAgIAkJ CSBvdXQgKyBpbiwgdnEtPm51bV9mcmVlKTsKPj4+Pj4gLQkJLyogV2Ugbm90aWZ5KmV2ZW4gaWYq ICBWUklOR19VU0VEX0ZfTk9fTk9USUZZIGlzIHNldCBoZXJlLiAqLwo+Pj4+PiAtCQl2cS0+bm90 aWZ5KCZ2cS0+dnEpOwo+Pj4+PiArCQkvKiBGSVhNRTogZm9yIGhpc3RvcmljYWwgcmVhc29ucywg d2UgZm9yY2UgYSBub3RpZnkgaGVyZSBpZgo+Pj4+PiArCQkgKiB0aGVyZSBhcmUgb3V0Z29pbmcg cGFydHMgdG8gdGhlIGJ1ZmZlci4gIFByZXN1bWFibHkgdGhlCj4+Pj4+ICsJCSAqIGhvc3Qgc2hv dWxkIHNlcnZpY2UgdGhlIHJpbmcgQVNBUC4gKi8KPj4+Pj4gKwkJaWYgKG91dCkKPj4+Pj4gKwkJ CXZxLT5ub3RpZnkoJnZxLT52cSk7Cj4+Pj4+ICAgICAJCUVORF9VU0UodnEpOwo+Pj4+PiAgICAg CQlyZXR1cm4gLUVOT1NQQzsKPj4+Pj4gICAgIAl9Cj4+Pj4+Cj4+Pj4+Cl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fClZpcnR1YWxpemF0aW9uIG1haWxpbmcg bGlzdApWaXJ0dWFsaXphdGlvbkBsaXN0cy5saW51eC1mb3VuZGF0aW9uLm9yZwpodHRwczovL2xp c3RzLmxpbnV4Zm91bmRhdGlvbi5vcmcvbWFpbG1hbi9saXN0aW5mby92aXJ0dWFsaXphdGlvbg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-4950-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id 4F8E1985DAF for ; Fri, 9 Nov 2018 10:04:55 +0000 (UTC) References: <20180711022711.7090-1-tiwei.bie@intel.com> <20180711022711.7090-4-tiwei.bie@intel.com> <20181107123933-mutt-send-email-mst@kernel.org> <20181108013759.GA20591@debian> <2d46a41e-bc00-276a-e19a-105c9dffc75a@redhat.com> <20181108091337-mutt-send-email-mst@kernel.org> <21d6dbd9-8f78-6939-0e80-27b470aeb00a@redhat.com> <20181108225555-mutt-send-email-mst@kernel.org> From: Jason Wang Message-ID: Date: Fri, 9 Nov 2018 18:04:42 +0800 MIME-Version: 1.0 In-Reply-To: <20181108225555-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: [virtio-dev] Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support To: "Michael S. Tsirkin" Cc: Tiwei Bie , virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, virtio-dev@lists.oasis-open.org, wexu@redhat.com, jfreimann@redhat.com List-ID: On 2018/11/9 上午11:58, Michael S. Tsirkin wrote: > On Fri, Nov 09, 2018 at 10:25:28AM +0800, Jason Wang wrote: >> On 2018/11/8 下午10:14, Michael S. Tsirkin wrote: >>> On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote: >>>> On 2018/11/8 上午9:38, Tiwei Bie wrote: >>>>>>> + >>>>>>> + if (vq->vq.num_free < descs_used) { >>>>>>> + pr_debug("Can't add buf len %i - avail = %i\n", >>>>>>> + descs_used, vq->vq.num_free); >>>>>>> + /* FIXME: for historical reasons, we force a notify here if >>>>>>> + * there are outgoing parts to the buffer. Presumably the >>>>>>> + * host should service the ring ASAP. */ >>>>>> I don't think we have a reason to do this for packed ring. >>>>>> No historical baggage there, right? >>>>> Based on the original commit log, it seems that the notify here >>>>> is just an "optimization". But I don't quite understand what does >>>>> the "the heuristics which KVM uses" refer to. If it's safe to drop >>>>> this in packed ring, I'd like to do it. >>>> According to the commit log, it seems like a workaround of lguest networking >>>> backend. I agree to drop it, we should not have such burden. >>>> >>>> But we should notice that, with this removed, the compare between packed vs >>>> split is kind of unfair. >>> I don't think this ever triggers to be frank. When would it? >> >> I think it can happen e.g in the path of XDP transmission in >> __virtnet_xdp_xmit_one(): >> >> >>         err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC); >>         if (unlikely(err)) >>                 return -ENOSPC; /* Caller handle free/refcnt */ >> > I see. We used to do it for regular xmit but stopped > doing it. Is it fine for xdp then? There's no traffic control in XDP, so it was the only thing we can do. > >>>> Consider the removal of lguest support recently, >>>> maybe we can drop this for split ring as well? >>>> >>>> Thanks >>> If it's helpful, then for sure we can drop it for virtio 1. >>> Can you see any perf differences at all? With which device? >> >> I don't test but consider the case of XDP_TX in guest plus vhost_net in >> host. Since vhost_net is half duplex, it's pretty easier to trigger this >> condition. >> >> Thanks > Sounds reasonable. Worth testing before we change things though. Let me test and submit a patch. Thanks > >>>>> commit 44653eae1407f79dff6f52fcf594ae84cb165ec4 >>>>> Author: Rusty Russell >>>>> Date: Fri Jul 25 12:06:04 2008 -0500 >>>>> >>>>> virtio: don't always force a notification when ring is full >>>>> We force notification when the ring is full, even if the host has >>>>> indicated it doesn't want to know. This seemed like a good idea at >>>>> the time: if we fill the transmit ring, we should tell the host >>>>> immediately. >>>>> Unfortunately this logic also applies to the receiving ring, which is >>>>> refilled constantly. We should introduce real notification thesholds >>>>> to replace this logic. Meanwhile, removing the logic altogether breaks >>>>> the heuristics which KVM uses, so we use a hack: only notify if there are >>>>> outgoing parts of the new buffer. >>>>> Here are the number of exits with lguest's crappy network implementation: >>>>> Before: >>>>> network xmit 7859051 recv 236420 >>>>> After: >>>>> network xmit 7858610 recv 118136 >>>>> Signed-off-by: Rusty Russell >>>>> >>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >>>>> index 72bf8bc09014..21d9a62767af 100644 >>>>> --- a/drivers/virtio/virtio_ring.c >>>>> +++ b/drivers/virtio/virtio_ring.c >>>>> @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq, >>>>> if (vq->num_free < out + in) { >>>>> pr_debug("Can't add buf len %i - avail = %i\n", >>>>> out + in, vq->num_free); >>>>> - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */ >>>>> - vq->notify(&vq->vq); >>>>> + /* FIXME: for historical reasons, we force a notify here if >>>>> + * there are outgoing parts to the buffer. Presumably the >>>>> + * host should service the ring ASAP. */ >>>>> + if (out) >>>>> + vq->notify(&vq->vq); >>>>> END_USE(vq); >>>>> return -ENOSPC; >>>>> } >>>>> >>>>> --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org