All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] vhost: postpone ring addresses translations at kick time only
@ 2017-11-03 15:52 Maxime Coquelin
  2017-11-03 15:56 ` Maxime Coquelin
  2017-11-06 11:58 ` Yuanhan Liu
  0 siblings, 2 replies; 6+ messages in thread
From: Maxime Coquelin @ 2017-11-03 15:52 UTC (permalink / raw)
  To: dev, yliu, lei.a.yao; +Cc: mst, Maxime Coquelin

If multiple queue pairs are created but all are not used, the
device is never started, as unused queues aren't enabled and
their ring addresses aren't translated. The device is changed
to running state when all rings addresses are translated.

This patch fixes this by postponning rings addresses translation
at kick time unconditionnaly, VHOST_USER_F_PROTOCOL_FEATURES
being negotiated or not.

Reported-by: Lei Yao <lei.a.yao@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 1f6cba4b9..849755093 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -755,15 +755,12 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"vring kick idx:%d file:%d\n", file.index, file.fd);
 
-	/*
-	 * Interpret ring addresses only when ring is started and enabled.
-	 * This is now if protocol features aren't supported.
-	 */
-	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
-		*pdev = dev = translate_ring_addresses(dev, file.index);
-		if (!dev)
-			return;
-	}
+	/* Interpret ring addresses only when ring is started. */
+	dev = translate_ring_addresses(dev, file.index);
+	if (!dev)
+		return;
+
+	*pdev = dev;
 
 	vq = dev->virtqueue[file.index];
 
@@ -845,29 +842,15 @@ vhost_user_get_vring_base(struct virtio_net *dev,
  * enable the virtio queue pair.
  */
 static int
-vhost_user_set_vring_enable(struct virtio_net **pdev,
+vhost_user_set_vring_enable(struct virtio_net *dev,
 			    VhostUserMsg *msg)
 {
-	struct virtio_net *dev = *pdev;
 	int enable = (int)msg->payload.state.num;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"set queue enable: %d to qp idx: %d\n",
 		enable, msg->payload.state.index);
 
-	/*
-	 * Interpret ring addresses only when ring is started and enabled.
-	 * This is now if protocol features are supported.
-	 */
-	if (enable && (dev->features &
-				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
-		dev = translate_ring_addresses(dev, msg->payload.state.index);
-		if (!dev)
-			return -1;
-
-		*pdev = dev;
-	}
-
 	if (dev->notify_ops->vring_state_changed)
 		dev->notify_ops->vring_state_changed(dev->vid,
 				msg->payload.state.index, enable);
@@ -1315,7 +1298,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
-		vhost_user_set_vring_enable(&dev, &msg);
+		vhost_user_set_vring_enable(dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
 		vhost_user_send_rarp(dev, &msg);
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: postpone ring addresses translations at kick time only
  2017-11-03 15:52 [PATCH] vhost: postpone ring addresses translations at kick time only Maxime Coquelin
@ 2017-11-03 15:56 ` Maxime Coquelin
  2017-11-06  3:26   ` Yao, Lei A
  2017-11-06 11:58 ` Yuanhan Liu
  1 sibling, 1 reply; 6+ messages in thread
From: Maxime Coquelin @ 2017-11-03 15:56 UTC (permalink / raw)
  To: dev, yliu, lei.a.yao; +Cc: mst

Hi Lei,

On 11/03/2017 04:52 PM, Maxime Coquelin wrote:
> If multiple queue pairs are created but all are not used, the
> device is never started, as unused queues aren't enabled and
> their ring addresses aren't translated. The device is changed
> to running state when all rings addresses are translated.
> 
> This patch fixes this by postponning rings addresses translation
> at kick time unconditionnaly, VHOST_USER_F_PROTOCOL_FEATURES
> being negotiated or not.
> 
> Reported-by: Lei Yao<lei.a.yao@intel.com>
> Signed-off-by: Maxime Coquelin<maxime.coquelin@redhat.com>
> ---
>   lib/librte_vhost/vhost_user.c | 33 ++++++++-------------------------
>   1 file changed, 8 insertions(+), 25 deletions(-)

Could you confirm the patch fixes the issue on your side?

I tested below cases with and without IOMMU:
- Host DPDK queues = 1 / QEMU queues = 1 / Guest DPDK queues = 1
- Host DPDK queues = 2 / QEMU queues = 2 / Guest DPDK queues = 1
- Host DPDK queues = 2 / QEMU queues = 2 / Guest DPDK queues = 2

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: postpone ring addresses translations at kick time only
  2017-11-03 15:56 ` Maxime Coquelin
@ 2017-11-06  3:26   ` Yao, Lei A
  2017-11-06  8:12     ` Maxime Coquelin
  0 siblings, 1 reply; 6+ messages in thread
From: Yao, Lei A @ 2017-11-06  3:26 UTC (permalink / raw)
  To: Maxime Coquelin, dev, yliu; +Cc: mst

Hi, Maxime

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Friday, November 3, 2017 11:57 PM
> To: dev@dpdk.org; yliu@fridaylinux.org; Yao, Lei A <lei.a.yao@intel.com>
> Cc: mst@redhat.com
> Subject: Re: [PATCH] vhost: postpone ring addresses translations at kick time
> only
> 
> Hi Lei,
> 
> On 11/03/2017 04:52 PM, Maxime Coquelin wrote:
> > If multiple queue pairs are created but all are not used, the
> > device is never started, as unused queues aren't enabled and
> > their ring addresses aren't translated. The device is changed
> > to running state when all rings addresses are translated.
> >
> > This patch fixes this by postponning rings addresses translation
> > at kick time unconditionnaly, VHOST_USER_F_PROTOCOL_FEATURES
> > being negotiated or not.
> >
> > Reported-by: Lei Yao<lei.a.yao@intel.com>
> > Signed-off-by: Maxime Coquelin<maxime.coquelin@redhat.com>
> > ---
> >   lib/librte_vhost/vhost_user.c | 33 ++++++++-------------------------
> >   1 file changed, 8 insertions(+), 25 deletions(-)
> 
> Could you confirm the patch fixes the issue on your side?
> 
> I tested below cases with and without IOMMU:
> - Host DPDK queues = 1 / QEMU queues = 1 / Guest DPDK queues = 1
> - Host DPDK queues = 2 / QEMU queues = 2 / Guest DPDK queues = 1
> - Host DPDK queues = 2 / QEMU queues = 2 / Guest DPDK queues = 2
> 
> Thanks,
> Maxime


Thanks for your patch. I test my test cases with your patch based on v17.11-rc2,
It can fix my issue here. 

BRs
Lei

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: postpone ring addresses translations at kick time only
  2017-11-06  3:26   ` Yao, Lei A
@ 2017-11-06  8:12     ` Maxime Coquelin
  0 siblings, 0 replies; 6+ messages in thread
From: Maxime Coquelin @ 2017-11-06  8:12 UTC (permalink / raw)
  To: Yao, Lei A, dev, yliu; +Cc: mst



On 11/06/2017 04:26 AM, Yao, Lei A wrote:
> Hi, Maxime
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Friday, November 3, 2017 11:57 PM
>> To: dev@dpdk.org; yliu@fridaylinux.org; Yao, Lei A <lei.a.yao@intel.com>
>> Cc: mst@redhat.com
>> Subject: Re: [PATCH] vhost: postpone ring addresses translations at kick time
>> only
>>
>> Hi Lei,
>>
>> On 11/03/2017 04:52 PM, Maxime Coquelin wrote:
>>> If multiple queue pairs are created but all are not used, the
>>> device is never started, as unused queues aren't enabled and
>>> their ring addresses aren't translated. The device is changed
>>> to running state when all rings addresses are translated.
>>>
>>> This patch fixes this by postponning rings addresses translation
>>> at kick time unconditionnaly, VHOST_USER_F_PROTOCOL_FEATURES
>>> being negotiated or not.
>>>
>>> Reported-by: Lei Yao<lei.a.yao@intel.com>
>>> Signed-off-by: Maxime Coquelin<maxime.coquelin@redhat.com>
>>> ---
>>>    lib/librte_vhost/vhost_user.c | 33 ++++++++-------------------------
>>>    1 file changed, 8 insertions(+), 25 deletions(-)
>>
>> Could you confirm the patch fixes the issue on your side?
>>
>> I tested below cases with and without IOMMU:
>> - Host DPDK queues = 1 / QEMU queues = 1 / Guest DPDK queues = 1
>> - Host DPDK queues = 2 / QEMU queues = 2 / Guest DPDK queues = 1
>> - Host DPDK queues = 2 / QEMU queues = 2 / Guest DPDK queues = 2
>>
>> Thanks,
>> Maxime
> 
> 
> Thanks for your patch. I test my test cases with your patch based on v17.11-rc2,
> It can fix my issue here.

Thanks for the testing!
Maxime

> BRs
> Lei
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: postpone ring addresses translations at kick time only
  2017-11-03 15:52 [PATCH] vhost: postpone ring addresses translations at kick time only Maxime Coquelin
  2017-11-03 15:56 ` Maxime Coquelin
@ 2017-11-06 11:58 ` Yuanhan Liu
  2017-11-07  1:28   ` Thomas Monjalon
  1 sibling, 1 reply; 6+ messages in thread
From: Yuanhan Liu @ 2017-11-06 11:58 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: dev, lei.a.yao, mst, Thomas Monjalon

On Fri, Nov 03, 2017 at 04:52:35PM +0100, Maxime Coquelin wrote:
> If multiple queue pairs are created but all are not used, the
> device is never started, as unused queues aren't enabled and
> their ring addresses aren't translated. The device is changed
> to running state when all rings addresses are translated.
> 
> This patch fixes this by postponning rings addresses translation
> at kick time unconditionnaly, VHOST_USER_F_PROTOCOL_FEATURES
> being negotiated or not.
> 
> Reported-by: Lei Yao <lei.a.yao@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Acked-by: Yuanhan Liu <yliu@fridaylinux.org>

	--yliu

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: postpone ring addresses translations at kick time only
  2017-11-06 11:58 ` Yuanhan Liu
@ 2017-11-07  1:28   ` Thomas Monjalon
  0 siblings, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2017-11-07  1:28 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: dev, Yuanhan Liu, lei.a.yao, mst

06/11/2017 12:58, Yuanhan Liu:
> On Fri, Nov 03, 2017 at 04:52:35PM +0100, Maxime Coquelin wrote:
> > If multiple queue pairs are created but all are not used, the
> > device is never started, as unused queues aren't enabled and
> > their ring addresses aren't translated. The device is changed
> > to running state when all rings addresses are translated.
> > 
> > This patch fixes this by postponning rings addresses translation
> > at kick time unconditionnaly, VHOST_USER_F_PROTOCOL_FEATURES
> > being negotiated or not.
> > 
> > Reported-by: Lei Yao <lei.a.yao@intel.com>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Acked-by: Yuanhan Liu <yliu@fridaylinux.org>

Applied, thanks

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-11-07  1:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-03 15:52 [PATCH] vhost: postpone ring addresses translations at kick time only Maxime Coquelin
2017-11-03 15:56 ` Maxime Coquelin
2017-11-06  3:26   ` Yao, Lei A
2017-11-06  8:12     ` Maxime Coquelin
2017-11-06 11:58 ` Yuanhan Liu
2017-11-07  1:28   ` Thomas Monjalon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.