[PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[abhimanyu.saini]

From: Abhimanyu Saini <absaini@amd.com>

libvhost calls dev_conf() before prosessing the
VHOST_USER_SET_VRING_CALL message for the last VQ. So
this message is processed after dev_conf() returns.

However, the dev_conf() function spawns a thread to set
rte_vhost_host_notifier_ctrl() before returning control to
libvhost. This parallel thread in turn invokes get_notify_area().
To get the notify_area, the vdpa driver needs to query the HW and
for this query it needs an enabled VQ.

But at the same time libvhost is processing the last
VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.

Hence there is a race b/w the libvhost and the vdpa driver.

To resolve this race condition, query the HW and cache notify_area
inside dev_conf() instead of doing it the parallel thread.

Signed-off-by: Abhimanyu Saini <absaini@amd.com>
---
 drivers/vdpa/sfc/sfc_vdpa_ops.c | 36 ++++++++++++++++++------------------
 drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/vdpa/sfc/sfc_vdpa_ops.c b/drivers/vdpa/sfc/sfc_vdpa_ops.c
index 63aa52d..b84699d 100644
--- a/drivers/vdpa/sfc/sfc_vdpa_ops.c
+++ b/drivers/vdpa/sfc/sfc_vdpa_ops.c
@@ -222,6 +222,7 @@
 sfc_vdpa_virtq_start(struct sfc_vdpa_ops_data *ops_data, int vq_num)
 {
 	int rc;
+	uint32_t doorbell;
 	efx_virtio_vq_t *vq;
 	struct sfc_vdpa_vring_info vring;
 	efx_virtio_vq_cfg_t vq_cfg;
@@ -270,22 +271,35 @@
 	/* Start virtqueue */
 	rc = efx_virtio_qstart(vq, &vq_cfg, &vq_dyncfg);
 	if (rc != 0) {
-		/* destroy virtqueue */
 		sfc_vdpa_err(ops_data->dev_handle,
 			     "virtqueue start failed: %s",
 			     rte_strerror(rc));
-		efx_virtio_qdestroy(vq);
 		goto fail_virtio_qstart;
 	}
 
 	sfc_vdpa_info(ops_data->dev_handle,
 		      "virtqueue started successfully for vq_num %d", vq_num);
 
+	rc = efx_virtio_get_doorbell_offset(vq,	&doorbell);
+	if (rc != 0) {
+		sfc_vdpa_err(ops_data->dev_handle,
+			     "failed to get doorbell offset: %s",
+			     rte_strerror(rc));
+		goto fail_doorbell;
+	}
+
+	/*
+	 * Cache the bar_offset here for each VQ here, it will come
+	 * in handy when sfc_vdpa_get_notify_area() is invoked.
+	 */
+	ops_data->vq_cxt[vq_num].doorbell = (void *)(uintptr_t)doorbell;
 	ops_data->vq_cxt[vq_num].enable = B_TRUE;
 
 	return rc;
 
+fail_doorbell:
 fail_virtio_qstart:
+	efx_virtio_qdestroy(vq);
 fail_vring_info:
 	return rc;
 }
@@ -792,8 +806,6 @@
 	int ret;
 	efx_nic_t *nic;
 	int vfio_dev_fd;
-	efx_rc_t rc;
-	unsigned int bar_offset;
 	volatile void *doorbell;
 	struct rte_pci_device *pci_dev;
 	struct rte_vdpa_device *vdpa_dev;
@@ -824,19 +836,6 @@
 		return -1;
 	}
 
-	if (ops_data->vq_cxt[qid].enable != B_TRUE) {
-		sfc_vdpa_err(dev, "vq is not enabled");
-		return -1;
-	}
-
-	rc = efx_virtio_get_doorbell_offset(ops_data->vq_cxt[qid].vq,
-					    &bar_offset);
-	if (rc != 0) {
-		sfc_vdpa_err(dev, "failed to get doorbell offset: %s",
-			     rte_strerror(rc));
-		return rc;
-	}
-
 	reg.index = sfc_vdpa_adapter_by_dev_handle(dev)->mem_bar.esb_rid;
 	ret = ioctl(vfio_dev_fd, VFIO_DEVICE_GET_REGION_INFO, &reg);
 	if (ret != 0) {
@@ -845,7 +844,8 @@
 		return ret;
 	}
 
-	*offset = reg.offset + bar_offset;
+	/* Use bar_offset that was cached during sfc_vdpa_virtq_start() */
+	*offset = reg.offset + (uint64_t)ops_data->vq_cxt[qid].doorbell;
 
 	len = (1U << encp->enc_vi_window_shift) / 2;
 	if (len >= sysconf(_SC_PAGESIZE)) {
diff --git a/drivers/vdpa/sfc/sfc_vdpa_ops.h b/drivers/vdpa/sfc/sfc_vdpa_ops.h
index 6d790fd..9dbd5b8 100644
--- a/drivers/vdpa/sfc/sfc_vdpa_ops.h
+++ b/drivers/vdpa/sfc/sfc_vdpa_ops.h
@@ -35,6 +35,7 @@ struct sfc_vdpa_vring_info {
 };
 
 typedef struct sfc_vdpa_vq_context_s {
+	volatile void			*doorbell;
 	uint8_t				enable;
 	uint32_t			pidx;
 	uint32_t			cidx;
-- 
1.8.3.1

Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[Maxime Coquelin]

Hi Abhimanyu,

On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
> From: Abhimanyu Saini <absaini@amd.com>
> 
> libvhost calls dev_conf() before prosessing the
> VHOST_USER_SET_VRING_CALL message for the last VQ. So
> this message is processed after dev_conf() returns.
> 
> However, the dev_conf() function spawns a thread to set
> rte_vhost_host_notifier_ctrl() before returning control to
> libvhost. This parallel thread in turn invokes get_notify_area().
> To get the notify_area, the vdpa driver needs to query the HW and
> for this query it needs an enabled VQ.
> 
> But at the same time libvhost is processing the last
> VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
> 
> Hence there is a race b/w the libvhost and the vdpa driver.
> 
> To resolve this race condition, query the HW and cache notify_area
> inside dev_conf() instead of doing it the parallel thread.
> 
> Signed-off-by: Abhimanyu Saini <absaini@amd.com>
> ---
>   drivers/vdpa/sfc/sfc_vdpa_ops.c | 36 ++++++++++++++++++------------------
>   drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
>   2 files changed, 19 insertions(+), 18 deletions(-)

We are really late in the v22.07 release cycle.
Does this issue reproduces easily, i.e. is it a blocker if not applied
in v22.07?

How confident are you about this fix? If we take it in -rc4 and it
introduces a regression, we might not be able to fix it on time for
final v22.07.

Also, it misses the Fixes tag, and stable is not CC'ed (The driver was
introduced in v22.11).

Regards,
Maxime

> diff --git a/drivers/vdpa/sfc/sfc_vdpa_ops.c b/drivers/vdpa/sfc/sfc_vdpa_ops.c
> index 63aa52d..b84699d 100644
> --- a/drivers/vdpa/sfc/sfc_vdpa_ops.c
> +++ b/drivers/vdpa/sfc/sfc_vdpa_ops.c
> @@ -222,6 +222,7 @@
>   sfc_vdpa_virtq_start(struct sfc_vdpa_ops_data *ops_data, int vq_num)
>   {
>   	int rc;
> +	uint32_t doorbell;
>   	efx_virtio_vq_t *vq;
>   	struct sfc_vdpa_vring_info vring;
>   	efx_virtio_vq_cfg_t vq_cfg;
> @@ -270,22 +271,35 @@
>   	/* Start virtqueue */
>   	rc = efx_virtio_qstart(vq, &vq_cfg, &vq_dyncfg);
>   	if (rc != 0) {
> -		/* destroy virtqueue */
>   		sfc_vdpa_err(ops_data->dev_handle,
>   			     "virtqueue start failed: %s",
>   			     rte_strerror(rc));
> -		efx_virtio_qdestroy(vq);
>   		goto fail_virtio_qstart;
>   	}
>   
>   	sfc_vdpa_info(ops_data->dev_handle,
>   		      "virtqueue started successfully for vq_num %d", vq_num);
>   
> +	rc = efx_virtio_get_doorbell_offset(vq,	&doorbell);
> +	if (rc != 0) {
> +		sfc_vdpa_err(ops_data->dev_handle,
> +			     "failed to get doorbell offset: %s",
> +			     rte_strerror(rc));
> +		goto fail_doorbell;
> +	}
> +
> +	/*
> +	 * Cache the bar_offset here for each VQ here, it will come
> +	 * in handy when sfc_vdpa_get_notify_area() is invoked.
> +	 */
> +	ops_data->vq_cxt[vq_num].doorbell = (void *)(uintptr_t)doorbell;
>   	ops_data->vq_cxt[vq_num].enable = B_TRUE;
>   
>   	return rc;
>   
> +fail_doorbell:
>   fail_virtio_qstart:
> +	efx_virtio_qdestroy(vq);
>   fail_vring_info:
>   	return rc;
>   }
> @@ -792,8 +806,6 @@
>   	int ret;
>   	efx_nic_t *nic;
>   	int vfio_dev_fd;
> -	efx_rc_t rc;
> -	unsigned int bar_offset;
>   	volatile void *doorbell;
>   	struct rte_pci_device *pci_dev;
>   	struct rte_vdpa_device *vdpa_dev;
> @@ -824,19 +836,6 @@
>   		return -1;
>   	}
>   
> -	if (ops_data->vq_cxt[qid].enable != B_TRUE) {
> -		sfc_vdpa_err(dev, "vq is not enabled");
> -		return -1;
> -	}
> -
> -	rc = efx_virtio_get_doorbell_offset(ops_data->vq_cxt[qid].vq,
> -					    &bar_offset);
> -	if (rc != 0) {
> -		sfc_vdpa_err(dev, "failed to get doorbell offset: %s",
> -			     rte_strerror(rc));
> -		return rc;
> -	}
> -
>   	reg.index = sfc_vdpa_adapter_by_dev_handle(dev)->mem_bar.esb_rid;
>   	ret = ioctl(vfio_dev_fd, VFIO_DEVICE_GET_REGION_INFO, &reg);
>   	if (ret != 0) {
> @@ -845,7 +844,8 @@
>   		return ret;
>   	}
>   
> -	*offset = reg.offset + bar_offset;
> +	/* Use bar_offset that was cached during sfc_vdpa_virtq_start() */
> +	*offset = reg.offset + (uint64_t)ops_data->vq_cxt[qid].doorbell;
>   
>   	len = (1U << encp->enc_vi_window_shift) / 2;
>   	if (len >= sysconf(_SC_PAGESIZE)) {
> diff --git a/drivers/vdpa/sfc/sfc_vdpa_ops.h b/drivers/vdpa/sfc/sfc_vdpa_ops.h
> index 6d790fd..9dbd5b8 100644
> --- a/drivers/vdpa/sfc/sfc_vdpa_ops.h
> +++ b/drivers/vdpa/sfc/sfc_vdpa_ops.h
> @@ -35,6 +35,7 @@ struct sfc_vdpa_vring_info {
>   };
>   
>   typedef struct sfc_vdpa_vq_context_s {
> +	volatile void			*doorbell;
>   	uint8_t				enable;
>   	uint32_t			pidx;
>   	uint32_t			cidx;

Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[Maxime Coquelin]

On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
> From: Abhimanyu Saini <absaini@amd.com>
> 
> libvhost calls dev_conf() before prosessing the
> VHOST_USER_SET_VRING_CALL message for the last VQ. So
> this message is processed after dev_conf() returns.
> 
> However, the dev_conf() function spawns a thread to set
> rte_vhost_host_notifier_ctrl() before returning control to
> libvhost. This parallel thread in turn invokes get_notify_area().
> To get the notify_area, the vdpa driver needs to query the HW and
> for this query it needs an enabled VQ.
> 
> But at the same time libvhost is processing the last
> VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
> 
> Hence there is a race b/w the libvhost and the vdpa driver.
> 
> To resolve this race condition, query the HW and cache notify_area
> inside dev_conf() instead of doing it the parallel thread.
> 
> Signed-off-by: Abhimanyu Saini <absaini@amd.com>
> ---
>   drivers/vdpa/sfc/sfc_vdpa_ops.c | 36 ++++++++++++++++++------------------
>   drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
>   2 files changed, 19 insertions(+), 18 deletions(-)
> 

During today's Release status meeting, Andrew mentioned that this patch
has been for a log time already in your internal tree.

So it gives a bit of confidence in taking it in -rc4.

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[David Marchand]

Hello Abhimanyu, Vijay,

On Thu, Jul 7, 2022 at 2:38 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
> > From: Abhimanyu Saini <absaini@amd.com>
> >
> > libvhost calls dev_conf() before prosessing the
> > VHOST_USER_SET_VRING_CALL message for the last VQ. So
> > this message is processed after dev_conf() returns.
> >
> > However, the dev_conf() function spawns a thread to set
> > rte_vhost_host_notifier_ctrl() before returning control to
> > libvhost. This parallel thread in turn invokes get_notify_area().
> > To get the notify_area, the vdpa driver needs to query the HW and
> > for this query it needs an enabled VQ.
> >
> > But at the same time libvhost is processing the last
> > VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
> >
> > Hence there is a race b/w the libvhost and the vdpa driver.
> >
> > To resolve this race condition, query the HW and cache notify_area
> > inside dev_conf() instead of doing it the parallel thread.
> >
> > Signed-off-by: Abhimanyu Saini <absaini@amd.com>
> > ---
> >   drivers/vdpa/sfc/sfc_vdpa_ops.c | 36 ++++++++++++++++++------------------
> >   drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
> >   2 files changed, 19 insertions(+), 18 deletions(-)
> >
>
> During today's Release status meeting, Andrew mentioned that this patch
> has been for a log time already in your internal tree.
>
> So it gives a bit of confidence in taking it in -rc4.

- But it is neither reviewed, nor acked by the driver maintainer.

Vijay, as this driver maintainer, your opinion matters.
We are in rc4 stage and we merge only critical fixes now.
There won't be much time to test this fix once merged (and I am not
talking about fixing a regression).

Are you confident with this fix? is it required for the 22.07 release?

If we don't get an answer, the safer is to let those fixes slip to a
next release.


- Besides, I see there is a new fix for some sfc driver.
https://patches.dpdk.org/project/dpdk/patch/20220708073702.29391-1-asaini@xilinx.com/
The same questions will be asked.


-- 
David Marchand

RE: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[Srivastava, Vijay]

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Friday, July 8, 2022 1:43 PM
>To: abhimanyu.saini@xilinx.com; vsrivast@xilinx.com
>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; dev <dev@dpdk.org>;
>Xia, Chenbo <chenbo.xia@intel.com>; Andrew Rybchenko
><andrew.rybchenko@oktetlabs.ru>; Saini, Abhimanyu
><abhimanyu.saini@amd.com>
>Subject: Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf
>
>CAUTION: This message has originated from an External Source. Please use
>proper judgment and caution when opening attachments, clicking links, or
>responding to this email.
>
>
>Hello Abhimanyu, Vijay,
>
>On Thu, Jul 7, 2022 at 2:38 PM Maxime Coquelin
><maxime.coquelin@redhat.com> wrote:
>> On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
>> > From: Abhimanyu Saini <absaini@amd.com>
>> >
>> > libvhost calls dev_conf() before prosessing the
>> > VHOST_USER_SET_VRING_CALL message for the last VQ. So this message
>> > is processed after dev_conf() returns.
>> >
>> > However, the dev_conf() function spawns a thread to set
>> > rte_vhost_host_notifier_ctrl() before returning control to libvhost.
>> > This parallel thread in turn invokes get_notify_area().
>> > To get the notify_area, the vdpa driver needs to query the HW and
>> > for this query it needs an enabled VQ.
>> >
>> > But at the same time libvhost is processing the last
>> > VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
>> >
>> > Hence there is a race b/w the libvhost and the vdpa driver.
>> >
>> > To resolve this race condition, query the HW and cache notify_area
>> > inside dev_conf() instead of doing it the parallel thread.
>> >
>> > Signed-off-by: Abhimanyu Saini <absaini@amd.com>
>> > ---
>> >   drivers/vdpa/sfc/sfc_vdpa_ops.c | 36 ++++++++++++++++++------------------
>> >   drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
>> >   2 files changed, 19 insertions(+), 18 deletions(-)
>> >
>>
>> During today's Release status meeting, Andrew mentioned that this
>> patch has been for a log time already in your internal tree.
>>
>> So it gives a bit of confidence in taking it in -rc4.
>
>- But it is neither reviewed, nor acked by the driver maintainer.
>
>Vijay, as this driver maintainer, your opinion matters.
>We are in rc4 stage and we merge only critical fixes now.
>There won't be much time to test this fix once merged (and I am not talking
>about fixing a regression).
>
>Are you confident with this fix? 
Yes. 

>is it required for the 22.07 release?
It is not a blocker issue, but it would be good to have in this release. 

>If we don't get an answer, the safer is to let those fixes slip to a next release.
>
>
>- Besides, I see there is a new fix for some sfc driver.
>https://patches.dpdk.org/project/dpdk/patch/20220708073702.29391-1-
>asaini@xilinx.com/
>The same questions will be asked.
>
>
>--
>David Marchand

Acked-by: Vijay Srivastava <vijays@amd.com>

Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[Maxime Coquelin]

On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
> From: Abhimanyu Saini <absaini@amd.com>
> 
> libvhost calls dev_conf() before prosessing the
> VHOST_USER_SET_VRING_CALL message for the last VQ. So
> this message is processed after dev_conf() returns.
> 
> However, the dev_conf() function spawns a thread to set
> rte_vhost_host_notifier_ctrl() before returning control to
> libvhost. This parallel thread in turn invokes get_notify_area().
> To get the notify_area, the vdpa driver needs to query the HW and
> for this query it needs an enabled VQ.
> 
> But at the same time libvhost is processing the last
> VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
> 
> Hence there is a race b/w the libvhost and the vdpa driver.
> 
> To resolve this race condition, query the HW and cache notify_area
> inside dev_conf() instead of doing it the parallel thread.
> 
> Signed-off-by: Abhimanyu Saini <absaini@amd.com>
> ---
>   drivers/vdpa/sfc/sfc_vdpa_ops.c | 36 ++++++++++++++++++------------------
>   drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
>   2 files changed, 19 insertions(+), 18 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime

Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[Maxime Coquelin]

On 7/8/22 11:23, Maxime Coquelin wrote:
> 
> 
> On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
>> From: Abhimanyu Saini <absaini@amd.com>
>>
>> libvhost calls dev_conf() before prosessing the
>> VHOST_USER_SET_VRING_CALL message for the last VQ. So
>> this message is processed after dev_conf() returns.
>>
>> However, the dev_conf() function spawns a thread to set
>> rte_vhost_host_notifier_ctrl() before returning control to
>> libvhost. This parallel thread in turn invokes get_notify_area().
>> To get the notify_area, the vdpa driver needs to query the HW and
>> for this query it needs an enabled VQ.
>>
>> But at the same time libvhost is processing the last
>> VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
>>
>> Hence there is a race b/w the libvhost and the vdpa driver.
>>
>> To resolve this race condition, query the HW and cache notify_area
>> inside dev_conf() instead of doing it the parallel thread.
>>
>> Signed-off-by: Abhimanyu Saini <absaini@amd.com>
>> ---
>>   drivers/vdpa/sfc/sfc_vdpa_ops.c | 36 
>> ++++++++++++++++++------------------
>>   drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
>>   2 files changed, 19 insertions(+), 18 deletions(-)
>>
> 
> Applied to dpdk-next-virtio/main.

Sorry, I notice it is missing the Fixes tag, and cc'ing stable.
Can you confirm this is needed and provide the faulty commit?

Thanks,
Maxime

> Thanks,
> Maxime

RE: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[Saini, Abhimanyu]

[AMD Official Use Only - General]

Sorry Maxime, I forgot to copy stable.
Yes it's required, the faulty commit is : 630be406dcbfc26260e9d9688c40a381d0f012db

-----Original Message-----
From: Maxime Coquelin <maxime.coquelin@redhat.com> 
Sent: Friday, July 8, 2022 2:55 PM
To: abhimanyu.saini@xilinx.com; dev@dpdk.org
Cc: chenbo.xia@intel.com; andrew.rybchenko@oktetlabs.ru; Saini, Abhimanyu <abhimanyu.saini@amd.com>
Subject: Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[CAUTION: External Email]

On 7/8/22 11:23, Maxime Coquelin wrote:
>
>
> On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
>> From: Abhimanyu Saini <absaini@amd.com>
>>
>> libvhost calls dev_conf() before prosessing the
>> VHOST_USER_SET_VRING_CALL message for the last VQ. So
>> this message is processed after dev_conf() returns.
>>
>> However, the dev_conf() function spawns a thread to set
>> rte_vhost_host_notifier_ctrl() before returning control to
>> libvhost. This parallel thread in turn invokes get_notify_area().
>> To get the notify_area, the vdpa driver needs to query the HW and
>> for this query it needs an enabled VQ.
>>
>> But at the same time libvhost is processing the last
>> VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
>>
>> Hence there is a race b/w the libvhost and the vdpa driver.
>>
>> To resolve this race condition, query the HW and cache notify_area
>> inside dev_conf() instead of doing it the parallel thread.
>>
>> Signed-off-by: Abhimanyu Saini <absaini@amd.com>
>> ---
>>   drivers/vdpa/sfc/sfc_vdpa_ops.c | 36
>> ++++++++++++++++++------------------
>>   drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
>>   2 files changed, 19 insertions(+), 18 deletions(-)
>>
>
> Applied to dpdk-next-virtio/main.

Sorry, I notice it is missing the Fixes tag, and cc'ing stable.
Can you confirm this is needed and provide the faulty commit?

Thanks,
Maxime

> Thanks,
> Maxime

Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf

[Maxime Coquelin]

On 7/8/22 12:25, Saini, Abhimanyu wrote:
> [AMD Official Use Only - General]
> 
> Sorry Maxime, I forgot to copy stable.
> Yes it's required, the faulty commit is : 630be406dcbfc26260e9d9688c40a381d0f012db

Thanks, ammended the commit message and applied to
dpdk-next-virtio/main.

> 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, July 8, 2022 2:55 PM
> To: abhimanyu.saini@xilinx.com; dev@dpdk.org
> Cc: chenbo.xia@intel.com; andrew.rybchenko@oktetlabs.ru; Saini, Abhimanyu <abhimanyu.saini@amd.com>
> Subject: Re: [PATCH] vdpa/sfc: resolve race between libvhost and dev_conf
> 
> [CAUTION: External Email]
> 
> On 7/8/22 11:23, Maxime Coquelin wrote:
>>
>>
>> On 7/6/22 11:24, abhimanyu.saini@xilinx.com wrote:
>>> From: Abhimanyu Saini <absaini@amd.com>
>>>
>>> libvhost calls dev_conf() before prosessing the
>>> VHOST_USER_SET_VRING_CALL message for the last VQ. So
>>> this message is processed after dev_conf() returns.
>>>
>>> However, the dev_conf() function spawns a thread to set
>>> rte_vhost_host_notifier_ctrl() before returning control to
>>> libvhost. This parallel thread in turn invokes get_notify_area().
>>> To get the notify_area, the vdpa driver needs to query the HW and
>>> for this query it needs an enabled VQ.
>>>
>>> But at the same time libvhost is processing the last
>>> VHOST_USER_SET_VRING_CALL, and to do that it disables the last VQ.
>>>
>>> Hence there is a race b/w the libvhost and the vdpa driver.
>>>
>>> To resolve this race condition, query the HW and cache notify_area
>>> inside dev_conf() instead of doing it the parallel thread.
>>>
>>> Signed-off-by: Abhimanyu Saini <absaini@amd.com>
>>> ---
>>>    drivers/vdpa/sfc/sfc_vdpa_ops.c | 36
>>> ++++++++++++++++++------------------
>>>    drivers/vdpa/sfc/sfc_vdpa_ops.h |  1 +
>>>    2 files changed, 19 insertions(+), 18 deletions(-)
>>>
>>
>> Applied to dpdk-next-virtio/main.
> 
> Sorry, I notice it is missing the Fixes tag, and cc'ing stable.
> Can you confirm this is needed and provide the faulty commit?
> 
> Thanks,
> Maxime
> 
>> Thanks,
>> Maxime