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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.