On Thu, May 10, 2018 at 03:34:50PM +0800, Jason Wang wrote:> On 2018?05?10? 15:32, Jason Wang wrote: > > On 2018?04?25? 13:15, Tiwei Bie wrote: > > > +??? /* We're using some buffers from the free list. */ > > > +??? vq->vq.num_free -= descs_used; > > > + > > > +??? /* Update free pointer */ > > > +??? if (indirect) { > > > +??????? n = head + 1; > > > +??????? if (n >= vq->vring_packed.num) { > > > +??????????? n = 0; > > > +??????????? vq->wrap_counter ^= 1; > > > +??????? } > > > +??????? vq->next_avail_idx = n; > > > +??? } else > > > +??????? vq->next_avail_idx = i; > > > > During testing zerocopy (out of order completion), I found driver may > > submit two identical buffer id to vhost. So the above code may not work > > well. > > > > Consider the case that driver adds 3 buffer and virtqueue size is 8. > > > > a) id = 0,count = 2,next_avail = 2 > > > > b) id = 2,count = 4,next_avail = 2 > > next_avail should be 6 here. > > > > > c) id = 4,count = 2,next_avail = 0 > > > > id should be 6 here. > > Thanks > > > if packet b is done before packet a, driver may think buffer id 0 is > > available and try to use it if even if the real buffer 0 was not done. > > > > ThanksNice catch! Thanks a lot! I'll implement an ID allocator. Best regards, Tiwei Bie
On 2018?05?10? 16:56, Tiwei Bie wrote:> On Thu, May 10, 2018 at 03:34:50PM +0800, Jason Wang wrote: >> On 2018?05?10? 15:32, Jason Wang wrote: >>> On 2018?04?25? 13:15, Tiwei Bie wrote: >>>> +??? /* We're using some buffers from the free list. */ >>>> +??? vq->vq.num_free -= descs_used; >>>> + >>>> +??? /* Update free pointer */ >>>> +??? if (indirect) { >>>> +??????? n = head + 1; >>>> +??????? if (n >= vq->vring_packed.num) { >>>> +??????????? n = 0; >>>> +??????????? vq->wrap_counter ^= 1; >>>> +??????? } >>>> +??????? vq->next_avail_idx = n; >>>> +??? } else >>>> +??????? vq->next_avail_idx = i; >>> During testing zerocopy (out of order completion), I found driver may >>> submit two identical buffer id to vhost. So the above code may not work >>> well. >>> >>> Consider the case that driver adds 3 buffer and virtqueue size is 8. >>> >>> a) id = 0,count = 2,next_avail = 2 >>> >>> b) id = 2,count = 4,next_avail = 2 >> next_avail should be 6 here. >> >>> c) id = 4,count = 2,next_avail = 0 >>> >> id should be 6 here. >> >> Thanks >> >>> if packet b is done before packet a, driver may think buffer id 0 is >>> available and try to use it if even if the real buffer 0 was not done. >>> >>> Thanks > Nice catch! Thanks a lot! > I'll implement an ID allocator. > > Best regards, > Tiwei BieSounds good. Another similar issue is detac_buf_packed(). It did: ??????? for (j = 0; j < vq->desc_state[head].num; j++) { ??????????????? desc = &vq->vring_packed.desc[i]; ??????????????? vring_unmap_one_packed(vq, desc); ??????????????? i++; ??????????????? if (i >= vq->vring_packed.num) ??????????????????????? i = 0; ??????? } This probably won't work for out of order too and according to the spec: """ Driver needs to keep track of the size of the list corresponding to each buffer ID, to be able to skip to where the next used descriptor is written by the device. """ Looks like we should not depend on the descriptor ring. Thanks
On Thu, May 10, 2018 at 05:49:20PM +0800, Jason Wang wrote:> On 2018?05?10? 16:56, Tiwei Bie wrote: > > On Thu, May 10, 2018 at 03:34:50PM +0800, Jason Wang wrote: > > > On 2018?05?10? 15:32, Jason Wang wrote: > > > > On 2018?04?25? 13:15, Tiwei Bie wrote: > > > > > +??? /* We're using some buffers from the free list. */ > > > > > +??? vq->vq.num_free -= descs_used; > > > > > + > > > > > +??? /* Update free pointer */ > > > > > +??? if (indirect) { > > > > > +??????? n = head + 1; > > > > > +??????? if (n >= vq->vring_packed.num) { > > > > > +??????????? n = 0; > > > > > +??????????? vq->wrap_counter ^= 1; > > > > > +??????? } > > > > > +??????? vq->next_avail_idx = n; > > > > > +??? } else > > > > > +??????? vq->next_avail_idx = i; > > > > During testing zerocopy (out of order completion), I found driver may > > > > submit two identical buffer id to vhost. So the above code may not work > > > > well. > > > > > > > > Consider the case that driver adds 3 buffer and virtqueue size is 8. > > > > > > > > a) id = 0,count = 2,next_avail = 2 > > > > > > > > b) id = 2,count = 4,next_avail = 2 > > > next_avail should be 6 here. > > > > > > > c) id = 4,count = 2,next_avail = 0 > > > > > > > id should be 6 here. > > > > > > Thanks > > > > > > > if packet b is done before packet a, driver may think buffer id 0 is > > > > available and try to use it if even if the real buffer 0 was not done. > > > > > > > > Thanks > > Nice catch! Thanks a lot! > > I'll implement an ID allocator. > > > > Best regards, > > Tiwei Bie > > Sounds good. > > Another similar issue is detac_buf_packed(). It did: > > ??????? for (j = 0; j < vq->desc_state[head].num; j++) { > ??????????????? desc = &vq->vring_packed.desc[i]; > ??????????????? vring_unmap_one_packed(vq, desc); > ??????????????? i++; > ??????????????? if (i >= vq->vring_packed.num) > ??????????????????????? i = 0; > ??????? } > > This probably won't work for out of order too and according to the spec: > > """ > Driver needs to keep track of the size of the list corresponding to each > buffer ID, to be able to skip to where the next used descriptor is written > by the device. > """ > > Looks like we should not depend on the descriptor ring.Yeah, the previous ID allocation is too simple.. Let me fix it in the next version. Thanks!> > Thanks