From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2C90FA00C2; Thu, 13 Oct 2022 09:17:03 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D004542C94; Thu, 13 Oct 2022 09:17:02 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 2A2DA42C27 for ; Thu, 13 Oct 2022 09:17:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1665645419; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OB6xh1TZT/U7xUF0pcr8zr1OmzW9wrtQ9EOZLjDLZi0=; b=PM8fbodLZuTKMRP1Y7W1DhpLcD8qXHOIB9Tlti6lwqTNRlpHdYwMMuWok390yghyEMAutS ePD8GLHlKZh/VJBhkhIwedn6Io4XHWSTeTTFDXqI6Nd/8gBBhMZblRwVFf3IQTTrhae6j4 NkPIig1M9r1WsEEI5ElcXqDouiiq99o= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-553-MNXvazSfPIOUdCkHKg421w-1; Thu, 13 Oct 2022 03:16:58 -0400 X-MC-Unique: MNXvazSfPIOUdCkHKg421w-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8B5AF185A78B; Thu, 13 Oct 2022 07:16:57 +0000 (UTC) Received: from [10.39.208.19] (unknown [10.39.208.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 160FD10031E6; Thu, 13 Oct 2022 07:16:55 +0000 (UTC) Message-ID: Date: Thu, 13 Oct 2022 09:16:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH v3 7/8] vhost: vDPA blk device gets ready when any queue is ready To: "Xia, Chenbo" , "Pei, Andy" , "dev@dpdk.org" Cc: "Xu, Rosen" , "Huang, Wei" , "Cao, Gang" References: <1661229305-240952-2-git-send-email-andy.pei@intel.com> <1663308990-621-1-git-send-email-andy.pei@intel.com> <1663308990-621-8-git-send-email-andy.pei@intel.com> From: Maxime Coquelin In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 10/13/22 03:00, Xia, Chenbo wrote: >> -----Original Message----- >> From: Pei, Andy >> Sent: Wednesday, October 12, 2022 8:13 PM >> To: Xia, Chenbo ; dev@dpdk.org >> Cc: Xu, Rosen ; Huang, Wei ; Cao, >> Gang ; maxime.coquelin@redhat.com >> Subject: RE: [PATCH v3 7/8] vhost: vDPA blk device gets ready when any >> queue is ready >> >> Hi Chenbo, >> >> Thanks for your reply. >> My reply is inline. >> >>> -----Original Message----- >>> From: Xia, Chenbo >>> Sent: Wednesday, October 12, 2022 5:09 PM >>> To: Pei, Andy ; dev@dpdk.org >>> Cc: Xu, Rosen ; Huang, Wei ; >>> Cao, Gang ; maxime.coquelin@redhat.com >>> Subject: RE: [PATCH v3 7/8] vhost: vDPA blk device gets ready when any >>> queue is ready >>> >>>> -----Original Message----- >>>> From: Pei, Andy >>>> Sent: Friday, September 16, 2022 2:16 PM >>>> To: dev@dpdk.org >>>> Cc: Xia, Chenbo ; Xu, Rosen >>>> ; Huang, Wei ; Cao, Gang >>>> ; maxime.coquelin@redhat.com >>>> Subject: [PATCH v3 7/8] vhost: vDPA blk device gets ready when any >>>> queue is ready >>>> >>>> When boot from virtio blk device, seabios in QEMU only enables one >> queue. >>>> To work in this scenario, vDPA BLK device back-end conf_dev when any >>> >>> What is conf_dev? >>> >> I refer to >> /** Driver configure the device (Mandatory) */ >> int (*dev_conf)(int vid); >> So do you think I should use "configure device"? > > Yes. It will be better > >> >>>> queue is ready. >>>> >>>> Signed-off-by: Andy Pei >>>> Signed-off-by: Huang Wei >>>> --- >>>> lib/vhost/vhost_user.c | 51 >>>> ++++++++++++++++++++++++++++++++------------- >>>> ----- >>>> 1 file changed, 33 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c index >>>> 4ad28ba..9169cf5 100644 >>>> --- a/lib/vhost/vhost_user.c >>>> +++ b/lib/vhost/vhost_user.c >>>> @@ -1449,9 +1449,10 @@ >>>> } >>>> >>>> #define VIRTIO_BUILTIN_NUM_VQS_TO_BE_READY 2u >>>> +#define VIRTIO_BLK_NUM_VQS_TO_BE_READY 1u >>>> >>>> static int >>>> -virtio_is_ready(struct virtio_net *dev) >>>> +virtio_is_ready(struct virtio_net *dev, uint32_t vdpa_type) >>>> { >>>> struct vhost_virtqueue *vq; >>>> uint32_t i, nr_vring = dev->nr_vring; @@ -1462,13 +1463,20 @@ >>>> if (!dev->nr_vring) >>>> return 0; >>>> >>>> - if (dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) { >>>> - nr_vring = VIRTIO_BUILTIN_NUM_VQS_TO_BE_READY; >>>> - >>>> - if (dev->nr_vring < nr_vring) >>>> - return 0; >>>> + if (vdpa_type == RTE_VHOST_VDPA_DEVICE_TYPE_NET) { >>>> + if (dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) >>>> + nr_vring = >>> VIRTIO_BUILTIN_NUM_VQS_TO_BE_READY; >>>> + } else { >>>> + /* >>>> + * vdpa_type == RTE_VHOST_VDPA_DEVICE_TYPE_BLK >>>> + * is the only case currently >>>> + */ >>>> + nr_vring = VIRTIO_BLK_NUM_VQS_TO_BE_READY; >>> >>> You should consider the case when vdpa device is not there. Maybe you >> can >>> use int for vdpa_type, -1 for non-vdpa. >>> >> I init vdpa_type to 0; >> #define RTE_VHOST_VDPA_DEVICE_TYPE_NET 0 >> #define RTE_VHOST_VDPA_DEVICE_TYPE_BLK 1 >> And get_dev_type only return RTE_VHOST_VDPA_DEVICE_TYPE_BLK or >> RTE_VHOST_VDPA_DEVICE_TYPE_NET. >> I think if when vdpa device is not there, this code runs in the original >> way. >> Do you think use init vdpa_type to -1 is better? > > I was talking about readability, current way will be confusing. So adding > -1 will be better. The check could be if (type == blk) ... else ... as > Type 0/-1 has the same handling. Also, the vdpa_type can be obtained from dev, so instead of passing the vdpa_type as argument for the function, it should get fetched directly in virtio_is_ready(). Maxime > Thanks, > Chenbo > >> >> >>> Also note that below check is only needed for some cases. >>> >> Yes, I got it. I will fix it in next version. >>> Thanks, >>> Chenbo >>> >>>> } >>>> >>>> + if (dev->nr_vring < nr_vring) >>>> + return 0; >>>> + >>>> for (i = 0; i < nr_vring; i++) { >>>> vq = dev->virtqueue[i]; >>>> >>>> @@ -3167,7 +3175,25 @@ static int is_vring_iotlb(struct virtio_net >> *dev, >>>> if (unlock_required) >>>> vhost_user_unlock_all_queue_pairs(dev); >>>> >>>> - if (ret != 0 || !virtio_is_ready(dev)) >>>> + if (ret != 0) >>>> + goto out; >>>> + >>>> + vdpa_dev = dev->vdpa_dev; >>>> + if (vdpa_dev) { >>>> + if (vdpa_dev->ops->get_dev_type) { >>>> + ret = vdpa_dev->ops->get_dev_type(vdpa_dev, >>> &vdpa_type); >>>> + if (ret) { >>>> + VHOST_LOG_CONFIG(dev->ifname, ERR, >>>> + "failed to get vdpa dev type.\n"); >>>> + ret = -1; >>>> + goto out; >>>> + } >>>> + } else { >>>> + vdpa_type = RTE_VHOST_VDPA_DEVICE_TYPE_NET; >>>> + } >>>> + } >>>> + >>>> + if (!virtio_is_ready(dev, vdpa_type)) >>>> goto out; >>>> >>>> /* >>>> @@ -3181,20 +3207,9 @@ static int is_vring_iotlb(struct virtio_net >> *dev, >>>> dev->flags |= VIRTIO_DEV_RUNNING; >>>> } >>>> >>>> - vdpa_dev = dev->vdpa_dev; >>>> if (!vdpa_dev) >>>> goto out; >>>> >>>> - if (vdpa_dev->ops->get_dev_type) { >>>> - ret = vdpa_dev->ops->get_dev_type(vdpa_dev, &vdpa_type); >>>> - if (ret) { >>>> - VHOST_LOG_CONFIG(dev->ifname, ERR, "failed to >>> get vdpa >>>> dev type.\n"); >>>> - ret = -1; >>>> - goto out; >>>> - } >>>> - } else { >>>> - vdpa_type = RTE_VHOST_VDPA_DEVICE_TYPE_NET; >>>> - } >>>> if (vdpa_type == RTE_VHOST_VDPA_DEVICE_TYPE_BLK >>>> && request != VHOST_USER_SET_VRING_CALL) >>>> goto out; >>>> -- >>>> 1.8.3.1 >