From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: Junjie Chen <junjie.j.chen@intel.com>,
jianfeng.tan@intel.com, mtetsuyah@gmail.com
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] net/vhost: fix segfault when creating vdev dynamically
Date: Thu, 29 Mar 2018 15:16:01 +0200 [thread overview]
Message-ID: <df7b39f3-e5b9-66d0-35c6-40af6fdb318e@redhat.com> (raw)
In-Reply-To: <20180329153544.270488-1-junjie.j.chen@intel.com>
Hi Junjie,
Thanks for the patch, it looks like the right thing to do.
Please find my comments below:
On 03/29/2018 05:35 PM, Junjie Chen wrote:
> when creating vdev dynamically, vhost pmd driver start directly without
s/when/When/
s/start/starts/
> checking TX/RX queues ready or not, and thus cause segmentation fault when
s/ready/are ready/
s/cause/causes/
> vhost library accessing queues. This patch add flag to check whether queues
s/accessing/accesses/
s/add flag/adds a flag/
> setup or not, and add driver start call into dev_start to allow user start
s/setup/are setup/
s/add/adds/
s/start/ to start/
> it after setting up queue.
s/queue/queues/
>
> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> ---
> Changes in v2:
> - check queue status in new_device, create queue in dev_start if not setup yet
> drivers/net/vhost/rte_eth_vhost.c | 73 ++++++++++++++++++++++++++++-----------
> 1 file changed, 53 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 3aae01c39..41410fa5a 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -117,7 +117,9 @@ struct pmd_internal {
> char *dev_name;
> char *iface_name;
> uint16_t max_queues;
> + uint16_t vid;
> rte_atomic32_t started;
> + rte_atomic32_t once;
The name is a bit vague.
Also, I wonder if we need a new variable.
Couldn't we rely on dev_attached? In new_device(), only set
dev_attached=1 if queues are allocated. And in eth_dev_start(),
attach queues if !dev_attached and then set dev_attached to 1.
> };
>
> struct internal_list {
> @@ -580,21 +582,27 @@ new_device(int vid)
> eth_dev->data->numa_node = newnode;
> #endif
>
> - for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> - vq = eth_dev->data->rx_queues[i];
> - if (vq == NULL)
> - continue;
> - vq->vid = vid;
> - vq->internal = internal;
> - vq->port = eth_dev->data->port_id;
> - }
> - for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> - vq = eth_dev->data->tx_queues[i];
> - if (vq == NULL)
> - continue;
> - vq->vid = vid;
> - vq->internal = internal;
> - vq->port = eth_dev->data->port_id;
> + 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 = vid;
> + vq->internal = internal;
> + vq->port = eth_dev->data->port_id;
> + }
> + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> + vq = eth_dev->data->tx_queues[i];
> + if (!vq)
> + continue;
> + vq->vid = vid;
> + vq->internal = internal;
> + vq->port = eth_dev->data->port_id;
> + }
> + } else {
> + RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> + internal->vid = vid;
> + rte_atomic32_set(&internal->once, 0);
> }
>
> for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> @@ -605,7 +613,8 @@ 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);
> + if (likely(rte_atomic32_read(&internal->once) == 1))
> + update_queuing_status(eth_dev);
>
> RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
>
> @@ -770,12 +779,34 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
> }
>
> static int
> -eth_dev_start(struct rte_eth_dev *dev)
> +eth_dev_start(struct rte_eth_dev *eth_dev)
> {
> - struct pmd_internal *internal = dev->data->dev_private;
> + struct pmd_internal *internal = eth_dev->data->dev_private;
> + struct vhost_queue *vq;
> + int i;
> +
> + if (unlikely(rte_atomic32_read(&internal->once) == 0)) {
> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> + vq = eth_dev->data->rx_queues[i];
> + if (!vq)
> + continue;
> + vq->vid = internal->vid;
> + vq->internal = internal;
> + vq->port = eth_dev->data->port_id;
> + }
> + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> + vq = eth_dev->data->tx_queues[i];
> + if (!vq)
> + continue;
> + vq->vid = internal->vid;
> + vq->internal = internal;
> + vq->port = eth_dev->data->port_id;
> + }
I think you could avoid the code duplication by creating a new function
to setup queues:
void queue_setup(struct rte_eth_dev *eth_dev,
struct pmd_internal *internal)
{
int i;
for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
vq = eth_dev->data->rx_queues[i];
if (!vq)
continue;
vq->vid = internal->vid;
vq->internal = internal;
vq->port = eth_dev->data->port_id;
}
for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
vq = eth_dev->data->tx_queues[i];
if (!vq)
continue;
vq->vid = internal->vid;
vq->internal = internal;
vq->port = eth_dev->data->port_id;
}
}
> + rte_atomic32_set(&internal->once, 1);
Indentation looks wrong here.
> + }
>
> rte_atomic32_set(&internal->started, 1);
> - update_queuing_status(dev);
> + update_queuing_status(eth_dev);
>
> return 0;
> }
> @@ -786,7 +817,9 @@ eth_dev_stop(struct rte_eth_dev *dev)
> struct pmd_internal *internal = dev->data->dev_private;
>
> rte_atomic32_set(&internal->started, 0);
> - update_queuing_status(dev);
> +
> + if (likely(rte_atomic32_read(&internal->once) == 1))
> + update_queuing_status(dev);
> }
>
> static void
>
next prev parent reply other threads:[~2018-03-29 13:16 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-27 16:05 [dpdk-dev] [PATCH] " Junjie Chen
2018-03-27 8:56 ` Tan, Jianfeng
2018-03-27 9:02 ` Chen, Junjie J
2018-03-27 9:10 ` Tan, Jianfeng
2018-03-27 9:24 ` Chen, Junjie J
2018-03-27 9:42 ` Tan, Jianfeng
2018-03-27 10:18 ` Chen, Junjie J
2018-03-27 13:54 ` Tan, Jianfeng
2018-03-27 11:28 ` Maxime Coquelin
2018-03-27 14:01 ` Tan, Jianfeng
2018-03-29 12:35 ` Maxime Coquelin
2018-03-29 15:35 ` [dpdk-dev] [PATCH v2] " Junjie Chen
2018-03-29 13:16 ` Maxime Coquelin [this message]
2018-03-30 6:58 ` [dpdk-dev] [PATCH v3] " Junjie Chen
2018-03-30 7:32 ` Yang, Zhiyong
2018-03-30 7:36 ` Maxime Coquelin
2018-03-30 7:35 ` Maxime Coquelin
2018-03-30 7:43 ` Maxime Coquelin
2018-04-09 12:37 ` Jens Freimann
2018-04-10 8:11 ` Chen, Junjie J
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=df7b39f3-e5b9-66d0-35c6-40af6fdb318e@redhat.com \
--to=maxime.coquelin@redhat.com \
--cc=dev@dpdk.org \
--cc=jianfeng.tan@intel.com \
--cc=junjie.j.chen@intel.com \
--cc=mtetsuyah@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).