diff mbox series

[v4,1/2] net/tap: change queue fd to be pointers to process private

Message ID 1538476438-20891-1-git-send-email-rasland@mellanox.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [v4,1/2] net/tap: change queue fd to be pointers to process private |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Raslan Darawsheh Oct. 2, 2018, 10:34 a.m. UTC 
  change the fds for the queues to be pointers and add new process private
structure and make the queue fds point to it.

Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
---
 drivers/net/tap/rte_eth_tap.c | 63 ++++++++++++++++++++++++-------------------
 drivers/net/tap/rte_eth_tap.h |  9 +++++--
 drivers/net/tap/tap_intr.c    |  4 +--
 3 files changed, 44 insertions(+), 32 deletions(-)

Comments

Ferruh Yigit Oct. 3, 2018, 5:59 p.m. UTC | #1 
On 10/2/2018 11:34 AM, Raslan Darawsheh wrote:
> change the fds for the queues to be pointers and add new process private
> structure and make the queue fds point to it.
> 
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 63 ++++++++++++++++++++++++-------------------
>  drivers/net/tap/rte_eth_tap.h |  9 +++++--
>  drivers/net/tap/tap_intr.c    |  4 +--
>  3 files changed, 44 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index ad5ae98..8cc4552 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -64,6 +64,7 @@
>  
>  static struct rte_vdev_driver pmd_tap_drv;
>  static struct rte_vdev_driver pmd_tun_drv;
> +static struct pmd_process_private *process_private;

This is the handlers for eth_dev queues, this should be _per eth_dev_, you can't
have a single global variable for this, we will see the problems below.

<...>

> @@ -1633,6 +1634,10 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
>  		goto error_exit_nodev;
>  	}
>  
> +	process_private = (struct pmd_process_private *)
> +		rte_zmalloc_socket(tap_name, sizeof(struct pmd_process_private),
> +			RTE_CACHE_LINE_SIZE, dev->device->numa_node);

For each tap device, you overwrite the "process_private",
- So it has the address of last created tap device, we will come back this one
- and problem is how to free this back if an eth_dev removed? We lost references
except last one.

> +
>  	pmd = dev->data->dev_private;
>  	pmd->dev = dev;
>  	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> @@ -1669,8 +1674,10 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
>  	/* Presetup the fds to -1 as being not valid */
>  	pmd->ka_fd = -1;
>  	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> -		pmd->rxq[i].fd = -1;
> -		pmd->txq[i].fd = -1;
> +		process_private->rxq_fds[i] = -1;
> +		process_private->txq_fds[i] = -1;
> +		pmd->rxq[i].fd = &process_private->rxq_fds[i];
> +		pmd->txq[i].fd = &process_private->txq_fds[i];

for eth_dev, dev->data->dev_private->rxq[i].fd points to a memory address,
and remember "dev->data->dev_private" is shared between primary and secondary,

when secondary updates its rxq[i].fd to point its memory block, won't it corrupt
the primary???

I think redirection is not helping here, I am not sure to the this within the
shared memory, you need a not shared area.
diff mbox series

Patch

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index ad5ae98..8cc4552 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -64,6 +64,7 @@ 
 
 static struct rte_vdev_driver pmd_tap_drv;
 static struct rte_vdev_driver pmd_tun_drv;
+static struct pmd_process_private *process_private;
 
 static const char *valid_arguments[] = {
 	ETH_TAP_IFACE_ARG,
@@ -331,7 +332,7 @@  pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		uint16_t data_off = rte_pktmbuf_headroom(mbuf);
 		int len;
 
-		len = readv(rxq->fd, *rxq->iovecs,
+		len = readv(*rxq->fd, *rxq->iovecs,
 			    1 +
 			    (rxq->rxmode->offloads & DEV_RX_OFFLOAD_SCATTER ?
 			     rxq->nb_rx_desc : 1));
@@ -595,7 +596,7 @@  tap_write_mbufs(struct tx_queue *txq, uint16_t num_mbufs,
 			tap_tx_l4_cksum(l4_cksum, l4_phdr_cksum, l4_raw_cksum);
 
 		/* copy the tx frame data */
-		n = writev(txq->fd, iovecs, j);
+		n = writev(*txq->fd, iovecs, j);
 		if (n <= 0)
 			break;
 		(*num_packets)++;
@@ -976,13 +977,13 @@  tap_dev_close(struct rte_eth_dev *dev)
 	tap_flow_implicit_flush(internals, NULL);
 
 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
-		if (internals->rxq[i].fd != -1) {
-			close(internals->rxq[i].fd);
-			internals->rxq[i].fd = -1;
+		if (*internals->rxq[i].fd != -1) {
+			close(*internals->rxq[i].fd);
+			*internals->rxq[i].fd = -1;
 		}
-		if (internals->txq[i].fd != -1) {
-			close(internals->txq[i].fd);
-			internals->txq[i].fd = -1;
+		if (*internals->txq[i].fd != -1) {
+			close(*internals->txq[i].fd);
+			*internals->txq[i].fd = -1;
 		}
 	}
 
@@ -1007,9 +1008,9 @@  tap_rx_queue_release(void *queue)
 {
 	struct rx_queue *rxq = queue;
 
-	if (rxq && (rxq->fd > 0)) {
-		close(rxq->fd);
-		rxq->fd = -1;
+	if (rxq && rxq->fd && (*rxq->fd > 0)) {
+		close(*rxq->fd);
+		*rxq->fd = -1;
 		rte_pktmbuf_free(rxq->pool);
 		rte_free(rxq->iovecs);
 		rxq->pool = NULL;
@@ -1022,9 +1023,9 @@  tap_tx_queue_release(void *queue)
 {
 	struct tx_queue *txq = queue;
 
-	if (txq && (txq->fd > 0)) {
-		close(txq->fd);
-		txq->fd = -1;
+	if (txq && txq->fd && (*txq->fd > 0)) {
+		close(*txq->fd);
+		*txq->fd = -1;
 	}
 }
 
@@ -1214,13 +1215,13 @@  tap_setup_queue(struct rte_eth_dev *dev,
 	struct rte_gso_ctx *gso_ctx;
 
 	if (is_rx) {
-		fd = &rx->fd;
-		other_fd = &tx->fd;
+		fd = rx->fd;
+		other_fd = tx->fd;
 		dir = "rx";
 		gso_ctx = NULL;
 	} else {
-		fd = &tx->fd;
-		other_fd = &rx->fd;
+		fd = tx->fd;
+		other_fd = rx->fd;
 		dir = "tx";
 		gso_ctx = &tx->gso_ctx;
 	}
@@ -1331,7 +1332,7 @@  tap_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 
 	TAP_LOG(DEBUG, "  RX TUNTAP device name %s, qid %d on fd %d",
-		internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
+		internals->name, rx_queue_id, *internals->rxq[rx_queue_id].fd);
 
 	return 0;
 
@@ -1371,7 +1372,7 @@  tap_tx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 	TAP_LOG(DEBUG,
 		"  TX TUNTAP device name %s, qid %d on fd %d csum %s",
-		internals->name, tx_queue_id, internals->txq[tx_queue_id].fd,
+		internals->name, tx_queue_id, *internals->txq[tx_queue_id].fd,
 		txq->csum ? "on" : "off");
 
 	return 0;
@@ -1633,6 +1634,10 @@  eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		goto error_exit_nodev;
 	}
 
+	process_private = (struct pmd_process_private *)
+		rte_zmalloc_socket(tap_name, sizeof(struct pmd_process_private),
+			RTE_CACHE_LINE_SIZE, dev->device->numa_node);
+
 	pmd = dev->data->dev_private;
 	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
@@ -1669,8 +1674,10 @@  eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	/* Presetup the fds to -1 as being not valid */
 	pmd->ka_fd = -1;
 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
-		pmd->rxq[i].fd = -1;
-		pmd->txq[i].fd = -1;
+		process_private->rxq_fds[i] = -1;
+		process_private->txq_fds[i] = -1;
+		pmd->rxq[i].fd = &process_private->rxq_fds[i];
+		pmd->txq[i].fd = &process_private->txq_fds[i];
 	}
 
 	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
@@ -2089,13 +2096,13 @@  rte_pmd_tap_remove(struct rte_vdev_device *dev)
 		tap_nl_final(internals->nlsk_fd);
 	}
 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
-		if (internals->rxq[i].fd != -1) {
-			close(internals->rxq[i].fd);
-			internals->rxq[i].fd = -1;
+		if (*internals->rxq[i].fd != -1) {
+			close(*internals->rxq[i].fd);
+			*internals->rxq[i].fd = -1;
 		}
-		if (internals->txq[i].fd != -1) {
-			close(internals->txq[i].fd);
-			internals->txq[i].fd = -1;
+		if (*internals->txq[i].fd != -1) {
+			close(*internals->txq[i].fd);
+			*internals->txq[i].fd = -1;
 		}
 	}
 
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 44e2773..4146f5c 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -46,7 +46,7 @@  struct rx_queue {
 	struct rte_mempool *mp;         /* Mempool for RX packets */
 	uint32_t trigger_seen;          /* Last seen Rx trigger value */
 	uint16_t in_port;               /* Port ID */
-	int fd;
+	int *fd;
 	struct pkt_stats stats;         /* Stats for this RX queue */
 	uint16_t nb_rx_desc;            /* max number of mbufs available */
 	struct rte_eth_rxmode *rxmode;  /* RX features */
@@ -56,7 +56,7 @@  struct rx_queue {
 };
 
 struct tx_queue {
-	int fd;
+	int *fd;
 	int type;                       /* Type field - TUN|TAP */
 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
 	uint16_t csum:1;                /* Enable checksum offloading */
@@ -92,6 +92,11 @@  struct pmd_internals {
 	int ka_fd;                        /* keep-alive file descriptor */
 };
 
+struct pmd_process_private {
+	int rxq_fds[RTE_PMD_TAP_MAX_QUEUES];
+	int txq_fds[RTE_PMD_TAP_MAX_QUEUES];
+};
+
 /* tap_intr.c */
 
 int tap_rx_intr_vec_set(struct rte_eth_dev *dev, int set);
diff --git a/drivers/net/tap/tap_intr.c b/drivers/net/tap/tap_intr.c
index fc59018..a4b212d 100644
--- a/drivers/net/tap/tap_intr.c
+++ b/drivers/net/tap/tap_intr.c
@@ -71,7 +71,7 @@  tap_rx_intr_vec_install(struct rte_eth_dev *dev)
 		struct rx_queue *rxq = pmd->dev->data->rx_queues[i];
 
 		/* Skip queues that cannot request interrupts. */
-		if (!rxq || rxq->fd <= 0) {
+		if (!rxq || !rxq->fd || *rxq->fd <= 0) {
 			/* Use invalid intr_vec[] index to disable entry. */
 			intr_handle->intr_vec[i] =
 				RTE_INTR_VEC_RXTX_OFFSET +
@@ -79,7 +79,7 @@  tap_rx_intr_vec_install(struct rte_eth_dev *dev)
 			continue;
 		}
 		intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + count;
-		intr_handle->efds[count] = rxq->fd;
+		intr_handle->efds[count] = *rxq->fd;
 		count++;
 	}
 	if (!count)