From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 71EFD1BB85 for ; Thu, 12 Apr 2018 09:35:47 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1269E82FC4A2; Thu, 12 Apr 2018 07:35:47 +0000 (UTC) Received: from [10.36.112.36] (ovpn-112-36.ams2.redhat.com [10.36.112.36]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E7A05215CDC8; Thu, 12 Apr 2018 07:35:45 +0000 (UTC) To: "Chen, Junjie J" , "Tan, Jianfeng" , "mtetsuyah@gmail.com" Cc: "dev@dpdk.org" References: <1523443993-176139-1-git-send-email-junjie.j.chen@intel.com> <1523466152-181190-1-git-send-email-junjie.j.chen@intel.com> <75c33a22-9b4c-f293-07ee-daad99f3cad0@intel.com> <6288ba60-eebb-a743-aa1d-a61c32a07c58@redhat.com> From: Maxime Coquelin Message-ID: <2c907472-444b-e8b6-2437-6fdcbcc21979@redhat.com> Date: Thu, 12 Apr 2018 09:35:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 12 Apr 2018 07:35:47 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 12 Apr 2018 07:35:47 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state 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: Thu, 12 Apr 2018 07:35:47 -0000 On 04/12/2018 09:34 AM, Chen, Junjie J wrote: >> >> >> >> On 04/12/2018 09:21 AM, Tan, Jianfeng wrote: >>> >>> >>> On 4/12/2018 1:02 AM, Junjie Chen wrote: >>>> dev_start sets *dev_attached* after setup queues, this sets device to >>>> invalid state since no frontend is attached. Also destroy_device set >>>> *started* to zero which makes *allow_queuing* always zero until >>>> dev_start get called again. Actually, we should not determine queues >>>> existence by >>>> *dev_attached* but by queues pointers or other separated variable(s). >>>> >>>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev >>>> dynamically") >>>> >>>> Signed-off-by: Junjie Chen >>>> Tested-by: Jens Freimann >>> >>> Overall, looks great to me except a nit below. >>> >>> Reviewed-by: Jianfeng Tan >> >> Thanks Jianfeng, I can handle the small change while applying. >> >> Can you confirm that it is implied that the queue are already allocated, else we >> wouldn't find the internal resource and quit earlier (in case of eth_dev_close >> called twice for example)? > > That is required, otherwise it generate segfault if we close device before queue setup. For example we > execute following steps in testpmd: > 1. port attach > 2. ctrl+D Thanks for confirming Junjie, I will apply it as is then. Reviewed-by: Maxime Coquelin Thanks, Maxime >> >> Thanks, >> Maxime >> >>> >>>> --- >>>> Changes in v3: >>>> - remove useless log in queue status showing Changes in v2: >>>> - use started to determine vhost queues readiness >>>> - revert setting started to zero in destroy_device >>>>   drivers/net/vhost/rte_eth_vhost.c | 59 >>>> +++++++++++++++++++-------------------- >>>>   1 file changed, 29 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c >>>> b/drivers/net/vhost/rte_eth_vhost.c >>>> index 11b6076..e392d71 100644 >>>> --- a/drivers/net/vhost/rte_eth_vhost.c >>>> +++ b/drivers/net/vhost/rte_eth_vhost.c >>>> @@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev >> *dev) >>>>       unsigned int i; >>>>       int allow_queuing = 1; >>>> -    if (rte_atomic32_read(&internal->dev_attached) == 0) >>>> +    if (!dev->data->rx_queues || !dev->data->tx_queues) >>>>           return; >>>> -    if (rte_atomic32_read(&internal->started) == 0) >>>> +    if (rte_atomic32_read(&internal->started) == 0 || >>>> +        rte_atomic32_read(&internal->dev_attached) == 0) >>>>           allow_queuing = 0; >>>>       /* Wait until rx/tx_pkt_burst stops accessing vhost device */ >>>> @@ -607,13 +608,10 @@ new_device(int vid) >>>>   #endif >>>>       internal->vid = vid; >>>> -    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { >>>> +    if (rte_atomic32_read(&internal->started) == 1) >>>>           queue_setup(eth_dev, internal); >>>> -        rte_atomic32_set(&internal->dev_attached, 1); >>>> -    } else { >>>> -        RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n"); >>>> -        rte_atomic32_set(&internal->dev_attached, 0); >>>> -    } >>>> +    else >>>> +        RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n"); >>>>       for (i = 0; i < rte_vhost_get_vring_num(vid); i++) >>>>           rte_vhost_enable_guest_notification(vid, i, 0); @@ -622,6 >>>> +620,7 @@ new_device(int vid) >>>>       eth_dev->data->dev_link.link_status = ETH_LINK_UP; >>>> +    rte_atomic32_set(&internal->dev_attached, 1); >>>>       update_queuing_status(eth_dev); >>>>       RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); @@ >>>> -651,23 +650,24 @@ destroy_device(int vid) >>>>       eth_dev = list->eth_dev; >>>>       internal = eth_dev->data->dev_private; >>>> -    rte_atomic32_set(&internal->started, 0); >>>> -    update_queuing_status(eth_dev); >>>>       rte_atomic32_set(&internal->dev_attached, 0); >>>> +    update_queuing_status(eth_dev); >>>>       eth_dev->data->dev_link.link_status = ETH_LINK_DOWN; >>>> -    for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { >>>> -        vq = eth_dev->data->rx_queues[i]; >>>> -        if (vq == NULL) >>>> -            continue; >>>> -        vq->vid = -1; >>>> -    } >>>> -    for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { >>>> -        vq = eth_dev->data->tx_queues[i]; >>>> -        if (vq == NULL) >>>> -            continue; >>>> -        vq->vid = -1; >>>> +    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { >>>> +        for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { >>>> +            vq = eth_dev->data->rx_queues[i]; >>>> +            if (!vq) >>>> +                continue; >>>> +            vq->vid = -1; >>>> +        } >>>> +        for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { >>>> +            vq = eth_dev->data->tx_queues[i]; >>>> +            if (!vq) >>>> +                continue; >>>> +            vq->vid = -1; >>>> +        } >>>>       } >>>>       state = vring_states[eth_dev->data->port_id]; >>>> @@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev) >>>>   { >>>>       struct pmd_internal *internal = eth_dev->data->dev_private; >>>> -    if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) { >>>> -        queue_setup(eth_dev, internal); >>>> -        rte_atomic32_set(&internal->dev_attached, 1); >>>> -    } >>>> - >>>> +    queue_setup(eth_dev, internal); >>>>       rte_atomic32_set(&internal->started, 1); >>>>       update_queuing_status(eth_dev); @@ -836,10 +832,13 @@ >>>> eth_dev_close(struct rte_eth_dev *dev) >>>>       pthread_mutex_unlock(&internal_list_lock); >>>>       rte_free(list); >>>> -    for (i = 0; i < dev->data->nb_rx_queues; i++) >>>> -        rte_free(dev->data->rx_queues[i]); >>>> -    for (i = 0; i < dev->data->nb_tx_queues; i++) >>>> -        rte_free(dev->data->tx_queues[i]); >>>> +    if (dev->data->rx_queues) >>> >>> This is implied that rx_queues is already allocated. So I don't think >>> we need this. >>> >>>> +        for (i = 0; i < dev->data->nb_rx_queues; i++) >>>> +            rte_free(dev->data->rx_queues[i]); >>>> + >>>> +    if (dev->data->tx_queues) >>>> +        for (i = 0; i < dev->data->nb_tx_queues; i++) >>>> +            rte_free(dev->data->tx_queues[i]); >>>>       rte_free(dev->data->mac_addrs); >>>>       free(internal->dev_name); >>>