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 7CC751B5E7 for ; Mon, 16 Oct 2017 11:48:03 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 88E455D68C; Mon, 16 Oct 2017 09:48:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 88E455D68C Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=maxime.coquelin@redhat.com Received: from [10.36.112.40] (ovpn-112-40.ams2.redhat.com [10.36.112.40]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 420756027E; Mon, 16 Oct 2017 09:47:55 +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> <2DBBFF226F7CF64BAFCA79B681719D953A2B92CC@shsmsx102.ccr.corp.intel.com> <2DBBFF226F7CF64BAFCA79B681719D953A2B92F7@shsmsx102.ccr.corp.intel.com> From: Maxime Coquelin Message-ID: Date: Mon, 16 Oct 2017 11:47:53 +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: <2DBBFF226F7CF64BAFCA79B681719D953A2B92F7@shsmsx102.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.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 16 Oct 2017 09:48:02 +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: Mon, 16 Oct 2017 09:48:03 -0000 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)? Maxime > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yao, Lei A >> Sent: Monday, October 16, 2017 2:00 PM >> To: 'Maxime Coquelin' ; '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, Maxime >> >>> -----Original Message----- >>> From: Yao, Lei A >>> Sent: Friday, October 13, 2017 3:55 PM >>> To: Maxime Coquelin ; 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, 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. >>> >> I find another issue with this patch when I use V17.11-rc1. >> It will break the connection between vhost and virtio-net in VM. >> The link status of vhost-user port is always down. >> But virtio-pmd driver is still ok. >> >> My qemu version: 2.5 >> Guest OS: Ubuntu 16.04 >> Guest kernel: 4.4.0 >> >> Could you have a check on this issue? Thanks a lot! >> >> BRs >> Lei >> >>>> >>>> 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 >>>>>