Re: [PATCH v9 1/9] net/tap: do not duplicate fd's

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

On 4/26/2024 4:48 PM, Stephen Hemminger wrote:
> The TAP device can use same file descriptopr for both rx and tx queues.
>

s/descriptopr/descriptor/

> This allows up to 8 queues (versus 4) to be used with secondary process.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

> @@ -1141,19 +1132,18 @@ tap_dev_close(struct rte_eth_dev *dev)
>  	}
>  
>  	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> -		if (process_private->rxq_fds[i] != -1) {
> -			rxq = &internals->rxq[i];
> -			close(process_private->rxq_fds[i]);
> -			process_private->rxq_fds[i] = -1;
> -			tap_rxq_pool_free(rxq->pool);
> -			rte_free(rxq->iovecs);
> -			rxq->pool = NULL;
> -			rxq->iovecs = NULL;
> -		}
> -		if (process_private->txq_fds[i] != -1) {
> -			close(process_private->txq_fds[i]);
> -			process_private->txq_fds[i] = -1;
> -		}
> +		struct rx_queue *rxq = &internals->rxq[i];
> +
> +		if (process_private->fds[i] == -1)
> +			continue;
> +
> +		close(process_private->fds[i]);
> +		process_private->fds[i] = -1;
>

can 'tap_queue_close()' be used here? (probably with slight change)

<...>

> @@ -1482,52 +1480,34 @@ tap_setup_queue(struct rte_eth_dev *dev,
>  		uint16_t qid,
>  		int is_rx)
>  {
> -	int ret;
> -	int *fd;
> -	int *other_fd;
> -	const char *dir;
> +	int fd, ret;
>  	struct pmd_internals *pmd = dev->data->dev_private;
>  	struct pmd_process_private *process_private = dev->process_private;
>  	struct rx_queue *rx = &internals->rxq[qid];
>  	struct tx_queue *tx = &internals->txq[qid];
> -	struct rte_gso_ctx *gso_ctx;
> +	struct rte_gso_ctx *gso_ctx = NULL;
> +	const char *dir = is_rx ? "rx" : "tx";
>  
> -	if (is_rx) {
> -		fd = &process_private->rxq_fds[qid];
> -		other_fd = &process_private->txq_fds[qid];
> -		dir = "rx";
> -		gso_ctx = NULL;
> -	} else {
> -		fd = &process_private->txq_fds[qid];
> -		other_fd = &process_private->rxq_fds[qid];
> -		dir = "tx";
> +	if (is_rx)
>  		gso_ctx = &tx->gso_ctx;
>

As commented on other version of this patch, shouldn't this be:
`if (!is_rx)`

<...>

> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index fa50fe45d7..a78fd50cd4 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1595,8 +1595,9 @@ tap_flow_isolate(struct rte_eth_dev *dev,
>  	 * If netdevice is there, setup appropriate flow rules immediately.
>  	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
>  	 */
> -	if (!process_private->rxq_fds[0])
> +	if (process_private->fds[0] == -1)
>

change in the condition looks reasonable but not directly related with
the change, does it require its own patch?