Linux Virtualization - Jun 2020 - [PATCH RFC v8 02/11] vhost: use batched get_vq_desc version

On 2020/6/11 ??7:34, Michael S. Tsirkin wrote:
>   static void vhost_vq_free_iovecs(struct vhost_virtqueue *vq)
>   {
>   	kfree(vq->descs);
> @@ -394,6 +400,9 @@ static long vhost_dev_alloc_iovecs(struct vhost_dev
*dev)
>   	for (i = 0; i < dev->nvqs; ++i) {
>   		vq = dev->vqs[i];
>   		vq->max_descs = dev->iov_limit;
> +		if (vhost_vq_num_batch_descs(vq) < 0) {
> +			return -EINVAL;
> +		}

This check breaks vdpa which set iov_limit to zero. Consider iov_limit 
is meaningless to vDPA, I wonder we can skip the test when device 
doesn't use worker.

Thanks

On Wed, Jun 17, 2020 at 11:19:26AM +0800, Jason Wang
wrote:
> 
> On 2020/6/11 ??7:34, Michael S. Tsirkin wrote:
> >   static void vhost_vq_free_iovecs(struct vhost_virtqueue *vq)
> >   {
> >   	kfree(vq->descs);
> > @@ -394,6 +400,9 @@ static long vhost_dev_alloc_iovecs(struct
vhost_dev *dev)
> >   	for (i = 0; i < dev->nvqs; ++i) {
> >   		vq = dev->vqs[i];
> >   		vq->max_descs = dev->iov_limit;
> > +		if (vhost_vq_num_batch_descs(vq) < 0) {
> > +			return -EINVAL;
> > +		}
> 
> 
> This check breaks vdpa which set iov_limit to zero. Consider iov_limit is
> meaningless to vDPA, I wonder we can skip the test when device doesn't
use
> worker.
> 
> Thanks
It doesn't need iovecs at all, right?

-- 
MST

On 2020/6/23 ??12:00, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2020 at 11:19:26AM +0800, Jason Wang wrote:
>> On 2020/6/11 ??7:34, Michael S. Tsirkin wrote:
>>>    static void vhost_vq_free_iovecs(struct vhost_virtqueue *vq)
>>>    {
>>>    	kfree(vq->descs);
>>> @@ -394,6 +400,9 @@ static long vhost_dev_alloc_iovecs(struct
vhost_dev *dev)
>>>    	for (i = 0; i < dev->nvqs; ++i) {
>>>    		vq = dev->vqs[i];
>>>    		vq->max_descs = dev->iov_limit;
>>> +		if (vhost_vq_num_batch_descs(vq) < 0) {
>>> +			return -EINVAL;
>>> +		}
>> This check breaks vdpa which set iov_limit to zero. Consider iov_limit
is
>> meaningless to vDPA, I wonder we can skip the test when device
doesn't use
>> worker.
>>
>> Thanks
> It doesn't need iovecs at all, right?
>
> -- MST

Yes, so we may choose to bypass the iovecs as well.

Thanks

On 2020/6/23 ??3:00, Eugenio Perez Martin wrote:
> On Tue, Jun 23, 2020 at 4:51 AM Jason Wang <jasowang at redhat.com>
wrote:
>>
>> On 2020/6/23 ??12:00, Michael S. Tsirkin wrote:
>>> On Wed, Jun 17, 2020 at 11:19:26AM +0800, Jason Wang wrote:
>>>> On 2020/6/11 ??7:34, Michael S. Tsirkin wrote:
>>>>>     static void vhost_vq_free_iovecs(struct vhost_virtqueue
*vq)
>>>>>     {
>>>>>      kfree(vq->descs);
>>>>> @@ -394,6 +400,9 @@ static long
vhost_dev_alloc_iovecs(struct vhost_dev *dev)
>>>>>      for (i = 0; i < dev->nvqs; ++i) {
>>>>>              vq = dev->vqs[i];
>>>>>              vq->max_descs = dev->iov_limit;
>>>>> +           if (vhost_vq_num_batch_descs(vq) < 0) {
>>>>> +                   return -EINVAL;
>>>>> +           }
>>>> This check breaks vdpa which set iov_limit to zero. Consider
iov_limit is
>>>> meaningless to vDPA, I wonder we can skip the test when device
doesn't use
>>>> worker.
>>>>
>>>> Thanks
>>> It doesn't need iovecs at all, right?
>>>
>>> -- MST
>>
>> Yes, so we may choose to bypass the iovecs as well.
>>
>> Thanks
>>
> I think that the kmalloc_array returns ZERO_SIZE_PTR for all of them
> in that case, so I didn't bother to skip the kmalloc_array parts.
> Would you prefer to skip them all and let them NULL? Or have I
> misunderstood what you mean?

I'm ok with either approach, but my understanding is that Michael wants 
to skip them all.

Thanks

>
> Thanks!
>

On Tue, Jun 23, 2020 at 09:00:57AM +0200, Eugenio Perez Martin
wrote:
> On Tue, Jun 23, 2020 at 4:51 AM Jason Wang <jasowang at redhat.com>
wrote:
> >
> >
> > On 2020/6/23 ??12:00, Michael S. Tsirkin wrote:
> > > On Wed, Jun 17, 2020 at 11:19:26AM +0800, Jason Wang wrote:
> > >> On 2020/6/11 ??7:34, Michael S. Tsirkin wrote:
> > >>>    static void vhost_vq_free_iovecs(struct
vhost_virtqueue *vq)
> > >>>    {
> > >>>     kfree(vq->descs);
> > >>> @@ -394,6 +400,9 @@ static long
vhost_dev_alloc_iovecs(struct vhost_dev *dev)
> > >>>     for (i = 0; i < dev->nvqs; ++i) {
> > >>>             vq = dev->vqs[i];
> > >>>             vq->max_descs = dev->iov_limit;
> > >>> +           if (vhost_vq_num_batch_descs(vq) < 0) {
> > >>> +                   return -EINVAL;
> > >>> +           }
> > >> This check breaks vdpa which set iov_limit to zero. Consider
iov_limit is
> > >> meaningless to vDPA, I wonder we can skip the test when
device doesn't use
> > >> worker.
> > >>
> > >> Thanks
> > > It doesn't need iovecs at all, right?
> > >
> > > -- MST
> >
> >
> > Yes, so we may choose to bypass the iovecs as well.
> >
> > Thanks
> >
> 
> I think that the kmalloc_array returns ZERO_SIZE_PTR for all of them
> in that case, so I didn't bother to skip the kmalloc_array parts.
> Would you prefer to skip them all and let them NULL? Or have I
> misunderstood what you mean?
> 
> Thanks!
Sorry about being unclear. I just meant that it seems cleaner
to check for iov_limit being 0 not for worker thread.

-- 
MST