Jason Wang
2020-Jun-11 03:02 UTC
[PATCH RFC v6 02/11] vhost: use batched get_vq_desc version
On 2020/6/10 ??7:05, Michael S. Tsirkin wrote:>>> +EXPORT_SYMBOL_GPL(vhost_get_vq_desc); >>> /* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */ >>> void vhost_discard_vq_desc(struct vhost_virtqueue *vq, int n) >>> { >>> + unfetch_descs(vq); >>> vq->last_avail_idx -= n; >> So unfetch_descs() has decreased last_avail_idx. >> Can we fix this by letting unfetch_descs() return the number and then we can >> do: >> >> int d = unfetch_descs(vq); >> vq->last_avail_idx -= (n > d) ? n - d: 0; >> >> Thanks > That's intentional I think - we need both.Yes, but:> > Unfetch_descs drops the descriptors in the cache that were > *not returned to caller* through get_vq_desc. > > vhost_discard_vq_desc drops the ones that were returned through get_vq_desc. > > Did I miss anything?We could count some descriptors twice, consider the case e.g we only cache on descriptor: fetch_descs() ??? fetch_buf() ??? ??? last_avail_idx++; Then we want do discard it: vhost_discard_avail_buf(1) ??? unfetch_descs() ??? ??? last_avail_idx--; ??? last_avail_idx -= 1; Thanks
Michael S. Tsirkin
2020-Jun-11 09:06 UTC
[PATCH RFC v6 02/11] vhost: use batched get_vq_desc version
On Thu, Jun 11, 2020 at 11:02:57AM +0800, Jason Wang wrote:> > On 2020/6/10 ??7:05, Michael S. Tsirkin wrote: > > > > +EXPORT_SYMBOL_GPL(vhost_get_vq_desc); > > > > /* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */ > > > > void vhost_discard_vq_desc(struct vhost_virtqueue *vq, int n) > > > > { > > > > + unfetch_descs(vq); > > > > vq->last_avail_idx -= n; > > > So unfetch_descs() has decreased last_avail_idx. > > > Can we fix this by letting unfetch_descs() return the number and then we can > > > do: > > > > > > int d = unfetch_descs(vq); > > > vq->last_avail_idx -= (n > d) ? n - d: 0; > > > > > > Thanks > > That's intentional I think - we need both. > > > Yes, but: > > > > > > Unfetch_descs drops the descriptors in the cache that were > > *not returned to caller* through get_vq_desc. > > > > vhost_discard_vq_desc drops the ones that were returned through get_vq_desc. > > > > Did I miss anything? > > We could count some descriptors twice, consider the case e.g we only cache > on descriptor: > > fetch_descs() > ??? fetch_buf() > ??? ??? last_avail_idx++; > > Then we want do discard it: > vhost_discard_avail_buf(1) > ??? unfetch_descs() > ??? ??? last_avail_idx--; > ??? last_avail_idx -= 1; > > ThanksI don't think that happens. vhost_discard_avail_buf(1) is only called after get vhost_get_avail_buf. vhost_get_avail_buf increments first_desc. unfetch_descs only counts from first_desc to ndescs. If I'm wrong, could you show values of first_desc and ndescs in this scenario? -- MST
Jason Wang
2020-Jun-15 02:43 UTC
[PATCH RFC v6 02/11] vhost: use batched get_vq_desc version
On 2020/6/11 ??5:06, Michael S. Tsirkin wrote:> On Thu, Jun 11, 2020 at 11:02:57AM +0800, Jason Wang wrote: >> On 2020/6/10 ??7:05, Michael S. Tsirkin wrote: >>>>> +EXPORT_SYMBOL_GPL(vhost_get_vq_desc); >>>>> /* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */ >>>>> void vhost_discard_vq_desc(struct vhost_virtqueue *vq, int n) >>>>> { >>>>> + unfetch_descs(vq); >>>>> vq->last_avail_idx -= n; >>>> So unfetch_descs() has decreased last_avail_idx. >>>> Can we fix this by letting unfetch_descs() return the number and then we can >>>> do: >>>> >>>> int d = unfetch_descs(vq); >>>> vq->last_avail_idx -= (n > d) ? n - d: 0; >>>> >>>> Thanks >>> That's intentional I think - we need both. >> >> Yes, but: >> >> >>> Unfetch_descs drops the descriptors in the cache that were >>> *not returned to caller* through get_vq_desc. >>> >>> vhost_discard_vq_desc drops the ones that were returned through get_vq_desc. >>> >>> Did I miss anything? >> We could count some descriptors twice, consider the case e.g we only cache >> on descriptor: >> >> fetch_descs() >> ??? fetch_buf() >> ??? ??? last_avail_idx++; >> >> Then we want do discard it: >> vhost_discard_avail_buf(1) >> ??? unfetch_descs() >> ??? ??? last_avail_idx--; >> ??? last_avail_idx -= 1; >> >> Thanks > > I don't think that happens. vhost_discard_avail_buf(1) is only called > after get vhost_get_avail_buf. vhost_get_avail_buf increments > first_desc. unfetch_descs only counts from first_desc to ndescs. > > If I'm wrong, could you show values of first_desc and ndescs in this > scenario?You're right, fetch_descriptor could not be called directly from the device code. Thanks>
Apparently Analagous Threads
- [PATCH RFC v6 02/11] vhost: use batched get_vq_desc version
- [PATCH RFC v6 02/11] vhost: use batched get_vq_desc version
- [PATCH RFC v6 02/11] vhost: use batched get_vq_desc version
- [PATCH RFC v6 02/11] vhost: use batched get_vq_desc version
- [PATCH RFC v6 02/11] vhost: use batched get_vq_desc version