From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754559AbeEHJQN (ORCPT ); Tue, 8 May 2018 05:16:13 -0400 Received: from mga03.intel.com ([134.134.136.65]:5552 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754208AbeEHJP5 (ORCPT ); Tue, 8 May 2018 05:15:57 -0400 X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,377,1520924400"; d="scan'208";a="39545737" Date: Tue, 8 May 2018 17:16:28 +0800 From: Tiwei Bie To: Jason Wang Cc: "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, wexu@redhat.com, jfreimann@redhat.com Subject: Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring Message-ID: <20180508091628.d7jzpopqopq4abhy@debian> References: <20180502184015-mutt-send-email-mst@kernel.org> <20180503011116.qvoyblcpklinrk26@debian> <20180503044218-mutt-send-email-mst@kernel.org> <20180503020949.5u3qz32gsk33z6vk@debian> <9f0b4e37-63ff-42f9-f2e6-3747a19a0206@redhat.com> <20180503135430.lbtvn4p4lyu3ksqo@debian> <12ede490-f674-2b89-d639-266b5fe15466@redhat.com> <20180508064409.kcn6amhsxu7nkuuc@debian> <34f2c690-7cb2-f9ea-2ce9-40f4ccb594c9@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <34f2c690-7cb2-f9ea-2ce9-40f4ccb594c9@redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 08, 2018 at 03:16:53PM +0800, Jason Wang wrote: > On 2018年05月08日 14:44, Tiwei Bie wrote: > > On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: > > > On 2018年05月08日 11:05, Jason Wang wrote: > > > > > Because in virtqueue_enable_cb_delayed(), we may set an > > > > > event_off which is bigger than new and both of them have > > > > > wrapped. And in this case, although new is smaller than > > > > > event_off (i.e. the third param -- old), new shouldn't > > > > > add vq->num, and actually we are expecting a very big > > > > > idx diff. > > > > Yes, so to calculate distance correctly between event and new, we just > > > > need to compare the warp counter and return false if it doesn't match > > > > without the need to try to add vq.num here. > > > > > > > > Thanks > > > Sorry, looks like the following should work, we need add vq.num if > > > used_wrap_counter does not match: > > > > > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, > > >                       __u16 off_wrap, __u16 new, > > >                       __u16 old) > > > { > > >     bool wrap = off_wrap >> 15; > > >     int off = off_wrap & ~(1 << 15); > > >     __u16 d1, d2; > > > > > >     if (wrap != vq->used_wrap_counter) > > >         d1 = new + vq->num - off - 1; > > Just to draw your attention (maybe you have already > > noticed this). > > I miss this, thanks! > > > > > In this case (i.e. wrap != vq->used_wrap_counter), > > it's also possible that (off < new) is true. Because, > > > > when virtqueue_enable_cb_delayed_packed() is used, > > `off` is calculated in driver in a way like this: > > > > off = vq->last_used_idx + bufs; > > if (off >= vq->vring_packed.num) { > > off -= vq->vring_packed.num; > > wrap_counter ^= 1; > > } > > > > And when `new` (in vhost) is close to vq->num. The > > vq->last_used_idx + bufs (in driver) can be bigger > > than vq->vring_packed.num, and: > > > > 1. `off` will wrap; > > 2. wrap counters won't match; > > 3. off < new; > > > > And d1 (i.e. new + vq->num - off - 1) will be a value > > bigger than vq->num. I'm okay with this, although it's > > a bit weird. > > > So I'm considering something more compact by reusing vring_need_event() by > pretending a larger queue size and adding vq->num back when necessary: > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, >                       __u16 off_wrap, __u16 new, >                       __u16 old) > { >     bool wrap = vq->used_wrap_counter; If the wrap counter is obtained from the vq, I think `new` should also be obtained from the vq. Or the wrap counter should be carried in `new`. >     int off = off_wrap & ~(1 << 15); >     __u16 d1, d2; > >     if (new < old) { >         new += vq->num; >         wrap ^= 1; >     } > >     if (wrap != off_wrap >> 15) >         off += vq->num; When `new` and `old` wraps, and `off` doesn't wrap, wrap != (off_wrap >> 15) will be true. In this case, `off` is bigger than `new`, and what we should do is `off -= vq->num` instead of `off += vq->num`. Best regards, Tiwei Bie > >     return vring_need_event(off, new, old); > } > > > > > > Best regards, > > Tiwei Bie > > > > >     else > > >         d1 = new - off - 1; > > > > > >     if (new > old) > > >         d2 = new - old; > > >     else > > >         d2 = new + vq->num - old; > > > > > >     return d1 < d2; > > > } > > > > > > Thanks > > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tiwei Bie Subject: Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring Date: Tue, 8 May 2018 17:16:28 +0800 Message-ID: <20180508091628.d7jzpopqopq4abhy@debian> References: <20180502184015-mutt-send-email-mst@kernel.org> <20180503011116.qvoyblcpklinrk26@debian> <20180503044218-mutt-send-email-mst@kernel.org> <20180503020949.5u3qz32gsk33z6vk@debian> <9f0b4e37-63ff-42f9-f2e6-3747a19a0206@redhat.com> <20180503135430.lbtvn4p4lyu3ksqo@debian> <12ede490-f674-2b89-d639-266b5fe15466@redhat.com> <20180508064409.kcn6amhsxu7nkuuc@debian> <34f2c690-7cb2-f9ea-2ce9-40f4ccb594c9@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: "Michael S. Tsirkin" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, wexu@redhat.com To: Jason Wang Return-path: Content-Disposition: inline In-Reply-To: <34f2c690-7cb2-f9ea-2ce9-40f4ccb594c9@redhat.com> 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 T24gVHVlLCBNYXkgMDgsIDIwMTggYXQgMDM6MTY6NTNQTSArMDgwMCwgSmFzb24gV2FuZyB3cm90 ZToKPiBPbiAyMDE45bm0MDXmnIgwOOaXpSAxNDo0NCwgVGl3ZWkgQmllIHdyb3RlOgo+ID4gT24g VHVlLCBNYXkgMDgsIDIwMTggYXQgMDE6NDA6NDBQTSArMDgwMCwgSmFzb24gV2FuZyB3cm90ZToK PiA+ID4gT24gMjAxOOW5tDA15pyIMDjml6UgMTE6MDUsIEphc29uIFdhbmcgd3JvdGU6Cj4gPiA+ ID4gPiBCZWNhdXNlIGluIHZpcnRxdWV1ZV9lbmFibGVfY2JfZGVsYXllZCgpLCB3ZSBtYXkgc2V0 IGFuCj4gPiA+ID4gPiBldmVudF9vZmYgd2hpY2ggaXMgYmlnZ2VyIHRoYW4gbmV3IGFuZCBib3Ro IG9mIHRoZW0gaGF2ZQo+ID4gPiA+ID4gd3JhcHBlZC4gQW5kIGluIHRoaXMgY2FzZSwgYWx0aG91 Z2ggbmV3IGlzIHNtYWxsZXIgdGhhbgo+ID4gPiA+ID4gZXZlbnRfb2ZmIChpLmUuIHRoZSB0aGly ZCBwYXJhbSAtLSBvbGQpLCBuZXcgc2hvdWxkbid0Cj4gPiA+ID4gPiBhZGQgdnEtPm51bSwgYW5k IGFjdHVhbGx5IHdlIGFyZSBleHBlY3RpbmcgYSB2ZXJ5IGJpZwo+ID4gPiA+ID4gaWR4IGRpZmYu Cj4gPiA+ID4gWWVzLCBzbyB0byBjYWxjdWxhdGUgZGlzdGFuY2UgY29ycmVjdGx5IGJldHdlZW4g ZXZlbnQgYW5kIG5ldywgd2UganVzdAo+ID4gPiA+IG5lZWQgdG8gY29tcGFyZSB0aGUgd2FycCBj b3VudGVyIGFuZCByZXR1cm4gZmFsc2UgaWYgaXQgZG9lc24ndCBtYXRjaAo+ID4gPiA+IHdpdGhv dXQgdGhlIG5lZWQgdG8gdHJ5IHRvIGFkZCB2cS5udW0gaGVyZS4KPiA+ID4gPiAKPiA+ID4gPiBU aGFua3MKPiA+ID4gU29ycnksIGxvb2tzIGxpa2UgdGhlIGZvbGxvd2luZyBzaG91bGQgd29yaywg d2UgbmVlZCBhZGQgdnEubnVtIGlmCj4gPiA+IHVzZWRfd3JhcF9jb3VudGVyIGRvZXMgbm90IG1h dGNoOgo+ID4gPiAKPiA+ID4gc3RhdGljIGJvb2wgdmhvc3RfdnJpbmdfcGFja2VkX25lZWRfZXZl bnQoc3RydWN0IHZob3N0X3ZpcnRxdWV1ZSAqdnEsCj4gPiA+ICDCoMKgwqAgwqDCoMKgIMKgwqDC oCDCoMKgwqAgwqDCoMKgIMKgIF9fdTE2IG9mZl93cmFwLCBfX3UxNiBuZXcsCj4gPiA+ICDCoMKg wqAgwqDCoMKgIMKgwqDCoCDCoMKgwqAgwqDCoMKgIMKgIF9fdTE2IG9sZCkKPiA+ID4gewo+ID4g PiAgwqDCoMKgIGJvb2wgd3JhcCA9IG9mZl93cmFwID4+IDE1Owo+ID4gPiAgwqDCoMKgIGludCBv ZmYgPSBvZmZfd3JhcCAmIH4oMSA8PCAxNSk7Cj4gPiA+ICDCoMKgwqAgX191MTYgZDEsIGQyOwo+ ID4gPiAKPiA+ID4gIMKgwqDCoCBpZiAod3JhcCAhPSB2cS0+dXNlZF93cmFwX2NvdW50ZXIpCj4g PiA+ICDCoMKgwqAgwqDCoMKgIGQxID0gbmV3ICsgdnEtPm51bSAtIG9mZiAtIDE7Cj4gPiBKdXN0 IHRvIGRyYXcgeW91ciBhdHRlbnRpb24gKG1heWJlIHlvdSBoYXZlIGFscmVhZHkKPiA+IG5vdGlj ZWQgdGhpcykuCj4gCj4gSSBtaXNzIHRoaXMsIHRoYW5rcyEKPiAKPiA+IAo+ID4gSW4gdGhpcyBj YXNlIChpLmUuIHdyYXAgIT0gdnEtPnVzZWRfd3JhcF9jb3VudGVyKSwKPiA+IGl0J3MgYWxzbyBw b3NzaWJsZSB0aGF0IChvZmYgPCBuZXcpIGlzIHRydWUuIEJlY2F1c2UsCj4gPiAKPiA+IHdoZW4g dmlydHF1ZXVlX2VuYWJsZV9jYl9kZWxheWVkX3BhY2tlZCgpIGlzIHVzZWQsCj4gPiBgb2ZmYCBp cyBjYWxjdWxhdGVkIGluIGRyaXZlciBpbiBhIHdheSBsaWtlIHRoaXM6Cj4gPiAKPiA+IAlvZmYg PSB2cS0+bGFzdF91c2VkX2lkeCArIGJ1ZnM7Cj4gPiAJaWYgKG9mZiA+PSB2cS0+dnJpbmdfcGFj a2VkLm51bSkgewo+ID4gCQlvZmYgLT0gdnEtPnZyaW5nX3BhY2tlZC5udW07Cj4gPiAJCXdyYXBf Y291bnRlciBePSAxOwo+ID4gCX0KPiA+IAo+ID4gQW5kIHdoZW4gYG5ld2AgKGluIHZob3N0KSBp cyBjbG9zZSB0byB2cS0+bnVtLiBUaGUKPiA+IHZxLT5sYXN0X3VzZWRfaWR4ICsgYnVmcyAoaW4g ZHJpdmVyKSBjYW4gYmUgYmlnZ2VyCj4gPiB0aGFuIHZxLT52cmluZ19wYWNrZWQubnVtLCBhbmQ6 Cj4gPiAKPiA+IDEuIGBvZmZgIHdpbGwgd3JhcDsKPiA+IDIuIHdyYXAgY291bnRlcnMgd29uJ3Qg bWF0Y2g7Cj4gPiAzLiBvZmYgPCBuZXc7Cj4gPiAKPiA+IEFuZCBkMSAoaS5lLiBuZXcgKyB2cS0+ bnVtIC0gb2ZmIC0gMSkgd2lsbCBiZSBhIHZhbHVlCj4gPiBiaWdnZXIgdGhhbiB2cS0+bnVtLiBJ J20gb2theSB3aXRoIHRoaXMsIGFsdGhvdWdoIGl0J3MKPiA+IGEgYml0IHdlaXJkLgo+IAo+IAo+ IFNvIEknbSBjb25zaWRlcmluZyBzb21ldGhpbmcgbW9yZSBjb21wYWN0IGJ5IHJldXNpbmcgdnJp bmdfbmVlZF9ldmVudCgpIGJ5Cj4gcHJldGVuZGluZyBhIGxhcmdlciBxdWV1ZSBzaXplIGFuZCBh ZGRpbmcgdnEtPm51bSBiYWNrIHdoZW4gbmVjZXNzYXJ5Ogo+IAo+IHN0YXRpYyBib29sIHZob3N0 X3ZyaW5nX3BhY2tlZF9uZWVkX2V2ZW50KHN0cnVjdCB2aG9zdF92aXJ0cXVldWUgKnZxLAo+IMKg wqDCoCDCoMKgwqAgwqDCoMKgIMKgwqDCoCDCoMKgwqAgwqAgX191MTYgb2ZmX3dyYXAsIF9fdTE2 IG5ldywKPiDCoMKgwqAgwqDCoMKgIMKgwqDCoCDCoMKgwqAgwqDCoMKgIMKgIF9fdTE2IG9sZCkK PiB7Cj4gwqDCoMKgIGJvb2wgd3JhcCA9IHZxLT51c2VkX3dyYXBfY291bnRlcjsKCklmIHRoZSB3 cmFwIGNvdW50ZXIgaXMgb2J0YWluZWQgZnJvbSB0aGUgdnEsCkkgdGhpbmsgYG5ld2Agc2hvdWxk IGFsc28gYmUgb2J0YWluZWQgZnJvbQp0aGUgdnEuIE9yIHRoZSB3cmFwIGNvdW50ZXIgc2hvdWxk IGJlIGNhcnJpZWQKaW4gYG5ld2AuCgo+IMKgwqDCoCBpbnQgb2ZmID0gb2ZmX3dyYXAgJiB+KDEg PDwgMTUpOwo+IMKgwqDCoCBfX3UxNiBkMSwgZDI7Cj4gCj4gwqDCoMKgIGlmIChuZXcgPCBvbGQp IHsKPiDCoMKgwqAgwqDCoMKgIG5ldyArPSB2cS0+bnVtOwo+IMKgwqDCoCDCoMKgwqAgd3JhcCBe PSAxOwo+IMKgwqDCoCB9Cj4gCj4gwqDCoMKgIGlmICh3cmFwICE9IG9mZl93cmFwID4+IDE1KQo+ IMKgwqDCoCDCoMKgwqAgb2ZmICs9IHZxLT5udW07CgpXaGVuIGBuZXdgIGFuZCBgb2xkYCB3cmFw cywgYW5kIGBvZmZgIGRvZXNuJ3Qgd3JhcCwKd3JhcCAhPSAob2ZmX3dyYXAgPj4gMTUpIHdpbGwg YmUgdHJ1ZS4gSW4gdGhpcyBjYXNlLApgb2ZmYCBpcyBiaWdnZXIgdGhhbiBgbmV3YCwgYW5kIHdo YXQgd2Ugc2hvdWxkIGRvCmlzIGBvZmYgLT0gdnEtPm51bWAgaW5zdGVhZCBvZiBgb2ZmICs9IHZx LT5udW1gLgoKQmVzdCByZWdhcmRzLApUaXdlaSBCaWUKCj4gCj4gwqDCoMKgIHJldHVybiB2cmlu Z19uZWVkX2V2ZW50KG9mZiwgbmV3LCBvbGQpOwo+IH0KPiAKPiAKPiA+IAo+ID4gQmVzdCByZWdh cmRzLAo+ID4gVGl3ZWkgQmllCj4gPiAKPiA+ID4gIMKgwqDCoCBlbHNlCj4gPiA+ICDCoMKgwqAg wqDCoMKgIGQxID0gbmV3IC0gb2ZmIC0gMTsKPiA+ID4gCj4gPiA+ICDCoMKgwqAgaWYgKG5ldyA+ IG9sZCkKPiA+ID4gIMKgwqDCoCDCoMKgwqAgZDIgPSBuZXcgLSBvbGQ7Cj4gPiA+ICDCoMKgwqAg ZWxzZQo+ID4gPiAgwqDCoMKgIMKgwqDCoCBkMiA9IG5ldyArIHZxLT5udW0gLSBvbGQ7Cj4gPiA+ IAo+ID4gPiAgwqDCoMKgIHJldHVybiBkMSA8IGQyOwo+ID4gPiB9Cj4gPiA+IAo+ID4gPiBUaGFu a3MKPiA+ID4gCj4gCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fClZpcnR1YWxpemF0aW9uIG1haWxpbmcgbGlzdApWaXJ0dWFsaXphdGlvbkBsaXN0cy5saW51 eC1mb3VuZGF0aW9uLm9yZwpodHRwczovL2xpc3RzLmxpbnV4Zm91bmRhdGlvbi5vcmcvbWFpbG1h bi9saXN0aW5mby92aXJ0dWFsaXphdGlvbg==