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 E78041B62E for ; Fri, 13 Oct 2017 09:32:25 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2F4C560D1; Fri, 13 Oct 2017 07:32:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2F4C560D1 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.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 5F0805C543; Fri, 13 Oct 2017 07:32:19 +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> From: Maxime Coquelin Message-ID: <1105ba3f-1f6f-d8f2-0299-2d26158790b8@redhat.com> Date: Fri, 13 Oct 2017 09:32:18 +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: <2DBBFF226F7CF64BAFCA79B681719D953A2A66FC@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.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 13 Oct 2017 07:32:25 +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:32:26 -0000 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? 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 >