From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 1F5BA1B65B for ; Fri, 13 Oct 2017 09:56:49 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3ECB283F3E; Fri, 13 Oct 2017 07:56:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3ECB283F3E Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=maxime.coquelin@redhat.com Received: from [10.36.112.24] (ovpn-112-24.ams2.redhat.com [10.36.112.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 770EA18B97; Fri, 13 Oct 2017 07:56:43 +0000 (UTC) To: "Yao, Lei A" , "dev@dpdk.org" , "Horton, Remy" , "Bie, Tiwei" , "yliu@fridaylinux.org" Cc: "mst@redhat.com" , "jfreiman@redhat.com" , "vkaplans@redhat.com" , "jasowang@redhat.com" References: <20171005083627.27828-1-maxime.coquelin@redhat.com> <20171005083627.27828-16-maxime.coquelin@redhat.com> <2DBBFF226F7CF64BAFCA79B681719D953A2A66FC@SHSMSX151.ccr.corp.intel.com> <1105ba3f-1f6f-d8f2-0299-2d26158790b8@redhat.com> <2DBBFF226F7CF64BAFCA79B681719D953A2ADA3F@SHSMSX151.ccr.corp.intel.com> From: Maxime Coquelin Message-ID: <2412a05e-a3f3-8352-68ac-b14475d63b92@redhat.com> Date: Fri, 13 Oct 2017 09:56:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <2DBBFF226F7CF64BAFCA79B681719D953A2ADA3F@SHSMSX151.ccr.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 13 Oct 2017 07:56:49 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v3 15/19] vhost: postpone rings addresses translation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Oct 2017 07:56:50 -0000 On 10/13/2017 09:55 AM, Yao, Lei A wrote: > Hi, Maxime > >> -----Original Message----- >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] >> Sent: Friday, October 13, 2017 3:32 PM >> To: Yao, Lei A ; dev@dpdk.org; Horton, Remy >> ; Bie, Tiwei ; >> yliu@fridaylinux.org >> Cc: mst@redhat.com; jfreiman@redhat.com; vkaplans@redhat.com; >> jasowang@redhat.com >> Subject: Re: [dpdk-dev] [PATCH v3 15/19] vhost: postpone rings addresses >> translation >> >> Hi Lei, >> >> On 10/13/2017 03:47 AM, Yao, Lei A wrote: >>> Hi, Maxime >>> >>> After this commit, vhost/virtio loopback test will fail when >>> use the CPU on socket 1. >>> Error message like following during initialize: >>> VHOST_CONFIG: vring kick idx:0 file:20 >>> VHOST_CONFIG: reallocate vq from 0 to 1 node >>> VHOST_CONFIG: reallocate dev from 0 to 1 node >>> VHOST_CONFIG: (0) failed to find avail ring address. >>> VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM >>> VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE >>> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR >>> VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK >>> VHOST_CONFIG: vring kick idx:1 file:21 >>> VHOST_CONFIG: reallocate vq from 0 to 1 node >>> VHOST_CONFIG: (0) failed to find avail ring address. >>> >>> But if use CPU on socket 0. It still can work. Could you have a check on >>> this? Thanks a lot! >> >> Thanks for reporting the issue. It seems addr pointer still points to >> the old structure after the reallocation. This patch should fix the >> issue: >> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >> index 9acac6125..2416a0061 100644 >> --- a/lib/librte_vhost/vhost_user.c >> +++ b/lib/librte_vhost/vhost_user.c >> @@ -417,6 +417,7 @@ translate_ring_addresses(struct virtio_net *dev, int >> vq_index) >> >> dev = numa_realloc(dev, vq_index); >> vq = dev->virtqueue[vq_index]; >> + addr = &vq->ring_addrs; >> >> vq->avail = (struct vring_avail *)(uintptr_t)ring_addr_to_vva(dev, >> vq, addr->avail_user_addr, sizeof(struct >> vring_avail)); >> >> I only have access to single-socket machines at the moment, so I cannot >> reproduce the issue. >> Can you have a try? > With your new patch, it can work on socket 1 now. Thanks. Great, thanks for testing. I'll post the patch today. Thanks, Maxime >> >> Thanks, >> Maxime >> >>> Following is my cmd: >>> Vhost: testpmd -n 4 -c 0x3000000 --socket-mem 1024,1024 --file- >> prefix=vhost \ >>> --vdev 'net_vhost0,iface=vhost-net,queues=1,client=0' -- -i >>> Virtio-user: testpmd -n 4 -c 0xc00000 --socket-mem 1024,1024 --no-pci -- >> file-prefix=virtio \ >>> --vdev=net_virtio_user0,mac=00:01:02:03:04:05,path=./vhost- >> net \ >>> -- -i --txqflags=0xf01 --disable-hw-vlan-filter >>> >>> BRs >>> Lei >>> >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime >> Coquelin >>>> Sent: Thursday, October 5, 2017 4:36 PM >>>> To: dev@dpdk.org; Horton, Remy ; Bie, Tiwei >>>> ; yliu@fridaylinux.org >>>> Cc: mst@redhat.com; jfreiman@redhat.com; vkaplans@redhat.com; >>>> jasowang@redhat.com; Maxime Coquelin >> >>>> Subject: [dpdk-dev] [PATCH v3 15/19] vhost: postpone rings addresses >>>> translation >>>> >>>> This patch postpones rings addresses translations and checks, as >>>> addresses sent by the master shuld not be interpreted as long as >>>> ring is not started and enabled[0]. >>>> >>>> When protocol features aren't negotiated, the ring is started in >>>> enabled state, so the addresses translations are postponed to >>>> vhost_user_set_vring_kick(). >>>> Otherwise, it is postponed to when ring is enabled, in >>>> vhost_user_set_vring_enable(). >>>> >>>> [0]: http://lists.nongnu.org/archive/html/qemu-devel/2017- >>>> 05/msg04355.html >>>> >>>> Signed-off-by: Maxime Coquelin >>>> --- >>>> lib/librte_vhost/vhost.h | 1 + >>>> lib/librte_vhost/vhost_user.c | 69 >>>> ++++++++++++++++++++++++++++++++++--------- >>>> 2 files changed, 56 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h >>>> index 79351c66f..903da5db5 100644 >>>> --- a/lib/librte_vhost/vhost.h >>>> +++ b/lib/librte_vhost/vhost.h >>>> @@ -125,6 +125,7 @@ struct vhost_virtqueue { >>>> >>>> struct vring_used_elem *shadow_used_ring; >>>> uint16_t shadow_used_idx; >>>> + struct vhost_vring_addr ring_addrs; >>>> >>>> struct batch_copy_elem *batch_copy_elems; >>>> uint16_t batch_copy_nb_elems; >>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >>>> index f495dd36e..319867c65 100644 >>>> --- a/lib/librte_vhost/vhost_user.c >>>> +++ b/lib/librte_vhost/vhost_user.c >>>> @@ -356,6 +356,7 @@ 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; >>>> @@ -363,35 +364,50 @@ vhost_user_set_vring_addr(struct virtio_net >> *dev, >>>> VhostUserMsg *msg) >>>> /* 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)); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static struct virtio_net *translate_ring_addresses(struct virtio_net *dev, >>>> + int vq_index) >>>> +{ >>>> + struct vhost_virtqueue *vq = dev->virtqueue[vq_index]; >>>> + struct vhost_vring_addr *addr = &vq->ring_addrs; >>>> + >>>> /* The addresses are converted from QEMU virtual to Vhost virtual. >>>> */ >>>> vq->desc = (struct vring_desc *)(uintptr_t)qva_to_vva(dev, >>>> - msg->payload.addr.desc_user_addr); >>>> + addr->desc_user_addr); >>>> if (vq->desc == 0) { >>>> RTE_LOG(ERR, VHOST_CONFIG, >>>> "(%d) failed to find desc ring address.\n", >>>> dev->vid); >>>> - return -1; >>>> + return NULL; >>>> } >>>> >>>> - dev = numa_realloc(dev, msg->payload.addr.index); >>>> - vq = dev->virtqueue[msg->payload.addr.index]; >>>> + dev = numa_realloc(dev, vq_index); >>>> + vq = dev->virtqueue[vq_index]; >>>> >>>> vq->avail = (struct vring_avail *)(uintptr_t)qva_to_vva(dev, >>>> - msg->payload.addr.avail_user_addr); >>>> + addr->avail_user_addr); >>>> if (vq->avail == 0) { >>>> RTE_LOG(ERR, VHOST_CONFIG, >>>> "(%d) failed to find avail ring address.\n", >>>> dev->vid); >>>> - return -1; >>>> + return NULL; >>>> } >>>> >>>> vq->used = (struct vring_used *)(uintptr_t)qva_to_vva(dev, >>>> - msg->payload.addr.used_user_addr); >>>> + addr->used_user_addr); >>>> if (vq->used == 0) { >>>> RTE_LOG(ERR, VHOST_CONFIG, >>>> "(%d) failed to find used ring address.\n", >>>> dev->vid); >>>> - return -1; >>>> + return NULL; >>>> } >>>> >>>> if (vq->last_used_idx != vq->used->idx) { >>>> @@ -403,7 +419,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, >>>> VhostUserMsg *msg) >>>> vq->last_avail_idx = vq->used->idx; >>>> } >>>> >>>> - vq->log_guest_addr = msg->payload.addr.log_guest_addr; >>>> + vq->log_guest_addr = addr->log_guest_addr; >>>> >>>> LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n", >>>> dev->vid, vq->desc); >>>> @@ -414,7 +430,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, >>>> VhostUserMsg *msg) >>>> LOG_DEBUG(VHOST_CONFIG, "(%d) log_guest_addr: %" PRIx64 "\n", >>>> dev->vid, vq->log_guest_addr); >>>> >>>> - return 0; >>>> + return dev; >>>> } >>>> >>>> /* >>>> @@ -685,10 +701,11 @@ vhost_user_set_vring_call(struct virtio_net >> *dev, >>>> struct VhostUserMsg *pmsg) >>>> } >>>> >>>> static void >>>> -vhost_user_set_vring_kick(struct virtio_net *dev, struct VhostUserMsg >>>> *pmsg) >>>> +vhost_user_set_vring_kick(struct virtio_net **pdev, struct >> VhostUserMsg >>>> *pmsg) >>>> { >>>> struct vhost_vring_file file; >>>> struct vhost_virtqueue *vq; >>>> + struct virtio_net *dev = *pdev; >>>> >>>> file.index = pmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK; >>>> if (pmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK) >>>> @@ -698,6 +715,16 @@ vhost_user_set_vring_kick(struct virtio_net *dev, >>>> 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; >>>> + } >>>> + >>>> vq = dev->virtqueue[file.index]; >>>> >>>> /* >>>> @@ -778,15 +805,29 @@ vhost_user_get_vring_base(struct virtio_net >> *dev, >>>> * enable the virtio queue pair. >>>> */ >>>> static int >>>> -vhost_user_set_vring_enable(struct virtio_net *dev, >>>> +vhost_user_set_vring_enable(struct virtio_net **pdev, >>>> 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); >>>> @@ -1155,7 +1196,7 @@ vhost_user_msg_handler(int vid, int fd) >>>> break; >>>> >>>> case VHOST_USER_SET_VRING_KICK: >>>> - vhost_user_set_vring_kick(dev, &msg); >>>> + vhost_user_set_vring_kick(&dev, &msg); >>>> break; >>>> case VHOST_USER_SET_VRING_CALL: >>>> vhost_user_set_vring_call(dev, &msg); >>>> @@ -1174,7 +1215,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 >>>