From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v4 1/2] net/tap: change queue fd to be pointers to process private Date: Wed, 3 Oct 2018 18:59:09 +0100 Message-ID: References: <1538047196-13789-2-git-send-email-rasland@mellanox.com> <1538476438-20891-1-git-send-email-rasland@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Thomas Monjalon , "dev@dpdk.org" , Shahaf Shuler , Ori Kam To: Raslan Darawsheh , "keith.wiles@intel.com" Return-path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id AC6FB1B2AB for ; Wed, 3 Oct 2018 19:59:12 +0200 (CEST) In-Reply-To: <1538476438-20891-1-git-send-email-rasland@mellanox.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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.