From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 94E469B67 for ; Wed, 3 Oct 2018 19:27:20 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2018 10:27:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,336,1534834800"; d="scan'208";a="79599364" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.49]) ([10.237.221.49]) by orsmga006.jf.intel.com with ESMTP; 03 Oct 2018 10:27:08 -0700 To: "Wiles, Keith" , Raslan Darawsheh Cc: Thomas Monjalon , "dev@dpdk.org" , Shahaf Shuler , Ori Katz References: <20180720105742.12669-1-thomas@monjalon.net> <1538047196-13789-1-git-send-email-rasland@mellanox.com> From: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: <6d2b3acd-76fb-8931-b06d-a583ccdeb43d@intel.com> Date: Wed, 3 Oct 2018 18:27:08 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/tap: change queue fd to be pointers to process private X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Oct 2018 17:27:21 -0000 On 10/2/2018 1:58 PM, Wiles, Keith wrote: > > >> On Oct 2, 2018, at 5:30 AM, Raslan Darawsheh 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. This should work, it is another kind of redirection. But I think there are other problems, current implementation seems broken, I will comment more detailed to the patch. Briefly, rx_pkt_burst/tx_pkt_burst are not in eth_dev->data, but eth_dev because they need to be private to process (can't share function pointers across processes), this is the same problem. > > 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 >> Sent: Thursday, September 27, 2018 4:18 PM >> To: Raslan Darawsheh >> Cc: Thomas Monjalon ; dev@dpdk.org; Shahaf Shuler ; Ori Katz >> 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 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 >>> --- >>> 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 >