[dpdk-dev,v3,15/19] vhost: postpone rings addresses translation
Checks
Commit Message
On 10/16/2017 11:47 AM, Maxime Coquelin wrote:
> Hi Yao,
>
> On 10/16/2017 08:23 AM, Yao, Lei A wrote:
>> Hi, Maxime
>>
>> Add one comment:
>> This issue with virtio-net only occur when I use CPU on socket 1.
>
> Thanks for the report.
> I fail to reproduce for now.
>
> What is your qemu command line?
> Is it reproducible systematically when there is a NUMA reallocation
> (DPDK on socket 0, QEMU on socket 1)?
Nevermind, I just reproduced the (an?) issue.
The issue I reproduce is not linked to NUMA reallocation, but to
messages sequencing differences between QEMU versions.
So, I'm not 100% this is the same issue, as you mention it works fine
when using CPU socket 0.
The patch "vhost: postpone rings addresses translation" moves rings
addresses translation at either vring kick or enable time, depending
on whether protocol features are enabled or not. This has been done
because we must not interpret ring information as long as the vring is
not fully initialized.
While my patch works fine with recent QEMU version, it breaks with older
ones, like QEMU v2.5. The reason is that on these older versions,
VHOST_USER_SET_VRING_ENABLE is called once and before
VHOST_USER_SET_VRING_ADDR. At that time, the ring adresses aren't
available so the translation is not done. On recent QEMU versions,
we receive VHOST_USER_SET_VRING_ENABLE also after having received
the rings addresses, so it works fine.
The below fix consists in performing the rings addresses translation
also when handling VHOST_USER_SET_VRING_ADDR if ring has already been
enabled.
I'll post a formal patch later today or tomorrow morning after having
tested it more conscientiously. Let me know if it fixes your issue.
Thanks,
Maxime
translate_ring_addresses(struct virtio_net *dev, int vq_index)
{
@@ -464,6 +437,43 @@ translate_ring_addresses(struct virtio_net *dev,
int vq_index)
}
/*
+ * The virtio device sends us the desc, used and avail ring addresses.
+ * This function then converts these to our address space.
+ */
+static int
+vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
+{
+ struct vhost_virtqueue *vq;
+ struct vhost_vring_addr *addr = &msg->payload.addr;
+ struct virtio_net *dev = *pdev;
+
+ if (dev->mem == NULL)
+ return -1;
+
+ /* addr->index refers to the queue index. The txq 1, rxq is 0. */
+ vq = dev->virtqueue[msg->payload.addr.index];
+
+ /*
+ * Rings addresses should not be interpreted as long as the ring
is not
+ * started and enabled
+ */
+ memcpy(&vq->ring_addrs, addr, sizeof(*addr));
+
+ vring_invalidate(dev, vq);
+
+ if (vq->enabled && (dev->features &
+ (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
+ dev = translate_ring_addresses(dev,
msg->payload.state.index);
+ if (!dev)
+ return -1;
+
+ *pdev = dev;
+ }
+
+ return 0;
+}
+
+/*
* The virtio device sends us the available ring last used index.
*/
static int
@@ -1273,7 +1283,7 @@ vhost_user_msg_handler(int vid, int fd)
vhost_user_set_vring_num(dev, &msg);
break;
case VHOST_USER_SET_VRING_ADDR:
- vhost_user_set_vring_addr(dev, &msg);
+ vhost_user_set_vring_addr(&dev, &msg);
break;
case VHOST_USER_SET_VRING_BASE:
vhost_user_set_vring_base(dev, &msg);
Comments
Hi, Maxime
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Monday, October 16, 2017 6:48 PM
> To: Yao, Lei A <lei.a.yao@intel.com>; 'dev@dpdk.org' <dev@dpdk.org>;
> Horton, Remy <remy.horton@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>;
> 'yliu@fridaylinux.org' <yliu@fridaylinux.org>
> Cc: 'mst@redhat.com' <mst@redhat.com>; 'jfreiman@redhat.com'
> <jfreiman@redhat.com>; 'vkaplans@redhat.com' <vkaplans@redhat.com>;
> 'jasowang@redhat.com' <jasowang@redhat.com>
> Subject: Re: [dpdk-dev] [PATCH v3 15/19] vhost: postpone rings addresses
> translation
>
>
>
> On 10/16/2017 11:47 AM, Maxime Coquelin wrote:
> > Hi Yao,
> >
> > On 10/16/2017 08:23 AM, Yao, Lei A wrote:
> >> Hi, Maxime
> >>
> >> Add one comment:
> >> This issue with virtio-net only occur when I use CPU on socket 1.
> >
> > Thanks for the report.
> > I fail to reproduce for now.
> >
> > What is your qemu command line?
> > Is it reproducible systematically when there is a NUMA reallocation
> > (DPDK on socket 0, QEMU on socket 1)?
>
> Nevermind, I just reproduced the (an?) issue.
> The issue I reproduce is not linked to NUMA reallocation, but to
> messages sequencing differences between QEMU versions.
>
> So, I'm not 100% this is the same issue, as you mention it works fine
> when using CPU socket 0.
>
> The patch "vhost: postpone rings addresses translation" moves rings
> addresses translation at either vring kick or enable time, depending
> on whether protocol features are enabled or not. This has been done
> because we must not interpret ring information as long as the vring is
> not fully initialized.
>
> While my patch works fine with recent QEMU version, it breaks with older
> ones, like QEMU v2.5. The reason is that on these older versions,
> VHOST_USER_SET_VRING_ENABLE is called once and before
> VHOST_USER_SET_VRING_ADDR. At that time, the ring adresses aren't
> available so the translation is not done. On recent QEMU versions,
> we receive VHOST_USER_SET_VRING_ENABLE also after having received
> the rings addresses, so it works fine.
>
> The below fix consists in performing the rings addresses translation
> also when handling VHOST_USER_SET_VRING_ADDR if ring has already been
> enabled.
>
> I'll post a formal patch later today or tomorrow morning after having
> tested it more conscientiously. Let me know if it fixes your issue.
>
> Thanks,
> Maxime
>
Thanks for your quick fix. I try your patch and it can totally work at my side for virtio-net.
I tested it with Qemu 2.5~2.7, 2.10.
The previous info about it can work on numa 0 is a misleading info. Because
I use qemu 2.10 for some special test at that time. So it can work.
BRs
Lei
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 76c4eeca5..1f6cba4b9 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -372,33 +372,6 @@ ring_addr_to_vva(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
> return qva_to_vva(dev, ra);
> }
>
> -/*
> - * The virtio device sends us the desc, used and avail ring addresses.
> - * This function then converts these to our address space.
> - */
> -static int
> -vhost_user_set_vring_addr(struct virtio_net *dev, VhostUserMsg *msg)
> -{
> - struct vhost_virtqueue *vq;
> - struct vhost_vring_addr *addr = &msg->payload.addr;
> -
> - if (dev->mem == NULL)
> - return -1;
> -
> - /* addr->index refers to the queue index. The txq 1, rxq is 0. */
> - vq = dev->virtqueue[msg->payload.addr.index];
> -
> - /*
> - * Rings addresses should not be interpreted as long as the ring
> is not
> - * started and enabled
> - */
> - memcpy(&vq->ring_addrs, addr, sizeof(*addr));
> -
> - vring_invalidate(dev, vq);
> -
> - return 0;
> -}
> -
> static struct virtio_net *
> translate_ring_addresses(struct virtio_net *dev, int vq_index)
> {
> @@ -464,6 +437,43 @@ translate_ring_addresses(struct virtio_net *dev,
> int vq_index)
> }
>
> /*
> + * The virtio device sends us the desc, used and avail ring addresses.
> + * This function then converts these to our address space.
> + */
> +static int
> +vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
> +{
> + struct vhost_virtqueue *vq;
> + struct vhost_vring_addr *addr = &msg->payload.addr;
> + struct virtio_net *dev = *pdev;
> +
> + if (dev->mem == NULL)
> + return -1;
> +
> + /* addr->index refers to the queue index. The txq 1, rxq is 0. */
> + vq = dev->virtqueue[msg->payload.addr.index];
> +
> + /*
> + * Rings addresses should not be interpreted as long as the ring
> is not
> + * started and enabled
> + */
> + memcpy(&vq->ring_addrs, addr, sizeof(*addr));
> +
> + vring_invalidate(dev, vq);
> +
> + if (vq->enabled && (dev->features &
> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
> + dev = translate_ring_addresses(dev,
> msg->payload.state.index);
> + if (!dev)
> + return -1;
> +
> + *pdev = dev;
> + }
> +
> + return 0;
> +}
> +
> +/*
> * The virtio device sends us the available ring last used index.
> */
> static int
> @@ -1273,7 +1283,7 @@ vhost_user_msg_handler(int vid, int fd)
> vhost_user_set_vring_num(dev, &msg);
> break;
> case VHOST_USER_SET_VRING_ADDR:
> - vhost_user_set_vring_addr(dev, &msg);
> + vhost_user_set_vring_addr(&dev, &msg);
> break;
> case VHOST_USER_SET_VRING_BASE:
> vhost_user_set_vring_base(dev, &msg);
On 10/17/2017 03:24 AM, Yao, Lei A wrote:
> Hi, Maxime
>
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Monday, October 16, 2017 6:48 PM
>> To: Yao, Lei A <lei.a.yao@intel.com>; 'dev@dpdk.org' <dev@dpdk.org>;
>> Horton, Remy <remy.horton@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>;
>> 'yliu@fridaylinux.org' <yliu@fridaylinux.org>
>> Cc: 'mst@redhat.com' <mst@redhat.com>; 'jfreiman@redhat.com'
>> <jfreiman@redhat.com>; 'vkaplans@redhat.com' <vkaplans@redhat.com>;
>> 'jasowang@redhat.com' <jasowang@redhat.com>
>> Subject: Re: [dpdk-dev] [PATCH v3 15/19] vhost: postpone rings addresses
>> translation
>>
>>
>>
>> On 10/16/2017 11:47 AM, Maxime Coquelin wrote:
>>> Hi Yao,
>>>
>>> On 10/16/2017 08:23 AM, Yao, Lei A wrote:
>>>> Hi, Maxime
>>>>
>>>> Add one comment:
>>>> This issue with virtio-net only occur when I use CPU on socket 1.
>>>
>>> Thanks for the report.
>>> I fail to reproduce for now.
>>>
>>> What is your qemu command line?
>>> Is it reproducible systematically when there is a NUMA reallocation
>>> (DPDK on socket 0, QEMU on socket 1)?
>>
>> Nevermind, I just reproduced the (an?) issue.
>> The issue I reproduce is not linked to NUMA reallocation, but to
>> messages sequencing differences between QEMU versions.
>>
>> So, I'm not 100% this is the same issue, as you mention it works fine
>> when using CPU socket 0.
>>
>> The patch "vhost: postpone rings addresses translation" moves rings
>> addresses translation at either vring kick or enable time, depending
>> on whether protocol features are enabled or not. This has been done
>> because we must not interpret ring information as long as the vring is
>> not fully initialized.
>>
>> While my patch works fine with recent QEMU version, it breaks with older
>> ones, like QEMU v2.5. The reason is that on these older versions,
>> VHOST_USER_SET_VRING_ENABLE is called once and before
>> VHOST_USER_SET_VRING_ADDR. At that time, the ring adresses aren't
>> available so the translation is not done. On recent QEMU versions,
>> we receive VHOST_USER_SET_VRING_ENABLE also after having received
>> the rings addresses, so it works fine.
>>
>> The below fix consists in performing the rings addresses translation
>> also when handling VHOST_USER_SET_VRING_ADDR if ring has already been
>> enabled.
>>
>> I'll post a formal patch later today or tomorrow morning after having
>> tested it more conscientiously. Let me know if it fixes your issue.
>>
>> Thanks,
>> Maxime
>>
> Thanks for your quick fix. I try your patch and it can totally work at my side for virtio-net.
> I tested it with Qemu 2.5~2.7, 2.10.
> The previous info about it can work on numa 0 is a misleading info. Because
> I use qemu 2.10 for some special test at that time. So it can work.
Great. Thanks for having tried it.
Maxime
> BRs
> Lei
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 76c4eeca5..1f6cba4b9 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -372,33 +372,6 @@ ring_addr_to_vva(struct virtio_net *dev, struct
>> vhost_virtqueue *vq,
>> return qva_to_vva(dev, ra);
>> }
>>
>> -/*
>> - * The virtio device sends us the desc, used and avail ring addresses.
>> - * This function then converts these to our address space.
>> - */
>> -static int
>> -vhost_user_set_vring_addr(struct virtio_net *dev, VhostUserMsg *msg)
>> -{
>> - struct vhost_virtqueue *vq;
>> - struct vhost_vring_addr *addr = &msg->payload.addr;
>> -
>> - if (dev->mem == NULL)
>> - return -1;
>> -
>> - /* addr->index refers to the queue index. The txq 1, rxq is 0. */
>> - vq = dev->virtqueue[msg->payload.addr.index];
>> -
>> - /*
>> - * Rings addresses should not be interpreted as long as the ring
>> is not
>> - * started and enabled
>> - */
>> - memcpy(&vq->ring_addrs, addr, sizeof(*addr));
>> -
>> - vring_invalidate(dev, vq);
>> -
>> - return 0;
>> -}
>> -
>> static struct virtio_net *
>> translate_ring_addresses(struct virtio_net *dev, int vq_index)
>> {
>> @@ -464,6 +437,43 @@ translate_ring_addresses(struct virtio_net *dev,
>> int vq_index)
>> }
>>
>> /*
>> + * The virtio device sends us the desc, used and avail ring addresses.
>> + * This function then converts these to our address space.
>> + */
>> +static int
>> +vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
>> +{
>> + struct vhost_virtqueue *vq;
>> + struct vhost_vring_addr *addr = &msg->payload.addr;
>> + struct virtio_net *dev = *pdev;
>> +
>> + if (dev->mem == NULL)
>> + return -1;
>> +
>> + /* addr->index refers to the queue index. The txq 1, rxq is 0. */
>> + vq = dev->virtqueue[msg->payload.addr.index];
>> +
>> + /*
>> + * Rings addresses should not be interpreted as long as the ring
>> is not
>> + * started and enabled
>> + */
>> + memcpy(&vq->ring_addrs, addr, sizeof(*addr));
>> +
>> + vring_invalidate(dev, vq);
>> +
>> + if (vq->enabled && (dev->features &
>> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
>> + dev = translate_ring_addresses(dev,
>> msg->payload.state.index);
>> + if (!dev)
>> + return -1;
>> +
>> + *pdev = dev;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> * The virtio device sends us the available ring last used index.
>> */
>> static int
>> @@ -1273,7 +1283,7 @@ vhost_user_msg_handler(int vid, int fd)
>> vhost_user_set_vring_num(dev, &msg);
>> break;
>> case VHOST_USER_SET_VRING_ADDR:
>> - vhost_user_set_vring_addr(dev, &msg);
>> + vhost_user_set_vring_addr(&dev, &msg);
>> break;
>> case VHOST_USER_SET_VRING_BASE:
>> vhost_user_set_vring_base(dev, &msg);
@@ -372,33 +372,6 @@ ring_addr_to_vva(struct virtio_net *dev, struct
vhost_virtqueue *vq,
return qva_to_vva(dev, ra);
}
-/*
- * The virtio device sends us the desc, used and avail ring addresses.
- * This function then converts these to our address space.
- */
-static int
-vhost_user_set_vring_addr(struct virtio_net *dev, VhostUserMsg *msg)
-{
- struct vhost_virtqueue *vq;
- struct vhost_vring_addr *addr = &msg->payload.addr;
-
- if (dev->mem == NULL)
- return -1;
-
- /* addr->index refers to the queue index. The txq 1, rxq is 0. */
- vq = dev->virtqueue[msg->payload.addr.index];
-
- /*
- * Rings addresses should not be interpreted as long as the ring
is not
- * started and enabled
- */
- memcpy(&vq->ring_addrs, addr, sizeof(*addr));
-
- vring_invalidate(dev, vq);
-
- return 0;
-}
-
static struct virtio_net *