Re: [dpdk-dev] [PATCH v3 1/2] net/tap: change queue fd to be pointers to process private

["Wiles, Keith" <keith.wiles@intel.com>]

> On Oct 2, 2018, at 5:30 AM, Raslan Darawsheh <rasland@mellanox.com> wrote:
> 
> Hi,
> 
> what I'm really doing is simply do some private array for all the fd's that each process will allocate it separately which will allow that each process will be 
> able to access the fd's for the queues in order not to overwrite the shared ones and it's working for me this way.
> 
> Now coming to your comment I'm not sure I fully understand it but, what you are suggesting is to create an array which will be accessed by the process_id to store these fd's in it.
> As far as I know we don't have something as process_id in dpdk we only have the system process id which is not relevant for the array of fd's.
> 
> Please correct me if I'm wrong, 
> I think this way we'll be limiting the number of secondary processes to number of queues by tap.
> Meanwhile, in my solution we don't have such limitation.

I did not explain myself very well in the previous email let me try again.

Right now you have a pointer to an allocated piece of memory holding the FD’s  struct pmd_process_private *process_private; if we converted the pointer to an array of pmd_process_private structures using RTE_PMD_TAP_MAX_QUEUES as an index.

struct pmd_process_private {
	int rx_fd;
	int tx_fd;
};

static struct pmd_process_private private_process[RTE_PMD_TAP_MAX_QUEUES]; /* each process has its own array */

struct rx_queue *rxq = pmd->dev->data->rx_queues[i];

struct rx_queue {
	struct pmd_process_private *priv;	// rxq->priv = &private_process[rx_queue_idx];
. . .
};

rxq->priv->rx_fd; // instead of the *rxq->fd seems shorter but less clear to me

Anyway my email compiler is not very good and maybe this will not work.

We can leave the current design in place for now, till I have time to really look at doing a PoC for my suggestion.

> 
> Kindest regards,
> Raslan Darawsheh
> 
> -----Original Message-----
> From: Wiles, Keith <keith.wiles@intel.com> 
> Sent: Thursday, September 27, 2018 4:18 PM
> To: Raslan Darawsheh <rasland@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Ori Katz <orik@mellanox.com>
> Subject: Re: [PATCH v3 1/2] net/tap: change queue fd to be pointers to process private
> 
> 
> 
>> On Sep 27, 2018, at 6:19 AM, Raslan Darawsheh <rasland@mellanox.com> 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;
> 
> Maybe I do not see some minor point for making fd a pointer to fd when we could have an array of process_private[RTE_PMD_TAP_MAX_QUEUES] instead of a pointer type here. Then we do not need to allocate the memory each PMD and they would still have a private copy. Remove the array of rx/tx fds in the structure. This way it appears we can remove the code below that is making fd a pointer to fd. It just seems overly complex to me at the cost of a few more bytes of memory.
> 
> This would remove int fd; from the structure and add a pointer to the pid_process_private instead, which is private by default.
> 
> Did I miss some detail here that makes my comment wrong?
> 
>> 
>> 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)
>> --
>> 2.7.4
>> 
> 
> Regards,
> Keith
> 

Regards,
Keith