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.c...@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
