[dpdk-dev] [PATCH v2] net/vhost: fix segfault when creating vdev dynamically
Maxime Coquelin
maxime.coquelin at redhat.com
Thu Mar 29 15:16:01 CEST 2018
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 at 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
>
More information about the dev
mailing list