Re: [dpdk-dev] [PATCH] net/tap: avoid using SIGIO

["Morten Brørup" <mb@smartsharesystems.com>]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Wednesday, July 15, 2020 1:58 AM
> 
> SIGIO maybe used by application, instead choose another rt-signal.
> Linux allows any signal to be used for signal based IO.
> Search for an unused signal in the available rt-signal range.

Just an observation. Feel free to ignore at your convenience:

The problem is the same as for SIGIO if the application sets up its own signal handler after this, and uses some hardcoded rt-signal that happens to be the one found to be free.

Unless the application doesn't use a hardcoded rt-signal, but also searches for an unused one.

So perhaps the "search for unused rt-signal" should be exposed as a generic support function for the application (and this driver) to use.

> 
> Add more error checking for fcntl and signal handling.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> Resending original version, marked as PATCH now.
> 
>  drivers/net/tap/rte_eth_tap.c | 99 ++++++++++++++++++++++++-----------
>  1 file changed, 67 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c
> index 339f24bf82f3..b19e26ba0e65 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -134,7 +134,7 @@ tun_alloc(struct pmd_internals *pmd, int
> is_keepalive)
>  #ifdef IFF_MULTI_QUEUE
>  	unsigned int features;
>  #endif
> -	int fd;
> +	int fd, signo, flags;
> 
>  	memset(&ifr, 0, sizeof(struct ifreq));
> 
> @@ -199,52 +199,87 @@ tun_alloc(struct pmd_internals *pmd, int
> is_keepalive)
>  		}
>  	}
> 
> +	flags = fcntl(fd, F_GETFL);
> +	if (flags == -1) {
> +		TAP_LOG(WARNING,
> +			"Unable to get %s current flags\n",
> +			ifr.ifr_name);
> +		goto error;
> +	}
> +
>  	/* Always set the file descriptor to non-blocking */
> -	if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) {
> +	flags |= O_NONBLOCK;
> +	if (fcntl(fd, F_SETFL, flags) < 0) {
>  		TAP_LOG(WARNING,
>  			"Unable to set %s to nonblocking: %s",
>  			ifr.ifr_name, strerror(errno));
>  		goto error;
>  	}
> 
> -	/* Set up trigger to optimize empty Rx bursts */
> -	errno = 0;
> -	do {
> +	/* Find a free realtime signal */
> +	for (signo = SIGRTMIN + 1; signo < SIGRTMAX; signo++) {
>  		struct sigaction sa;
> -		int flags = fcntl(fd, F_GETFL);
> 
> -		if (flags == -1 || sigaction(SIGIO, NULL, &sa) == -1)
> +		if (sigaction(signo, NULL, &sa) == -1) {
> +			TAP_LOG(WARNING,
> +				"Unable to get current rt-signal %d handler",
> +				signo);
> +			goto error;
> +		}
> +
> +		/* Already have the handler we want on this signal  */
> +		if (sa.sa_handler == tap_trigger_cb)
>  			break;
> -		if (sa.sa_handler != tap_trigger_cb) {
> -			/*
> -			 * Make sure SIGIO is not already taken. This is done
> -			 * as late as possible to leave the application a
> -			 * chance to set up its own signal handler first.
> -			 */
> -			if (sa.sa_handler != SIG_IGN &&
> -			    sa.sa_handler != SIG_DFL) {
> -				errno = EBUSY;
> -				break;
> -			}
> -			sa = (struct sigaction){
> -				.sa_flags = SA_RESTART,
> -				.sa_handler = tap_trigger_cb,
> -			};
> -			if (sigaction(SIGIO, &sa, NULL) == -1)
> -				break;
> +
> +		/* Is handler in use by application */
> +		if (sa.sa_handler != SIG_DFL) {
> +			TAP_LOG(DEBUG,
> +				"Skipping used rt-signal %d", signo);
> +			continue;
>  		}
> -		/* Enable SIGIO on file descriptor */
> -		fcntl(fd, F_SETFL, flags | O_ASYNC);
> -		fcntl(fd, F_SETOWN, getpid());
> -	} while (0);
> 
> -	if (errno) {
> +		sa = (struct sigaction) {
> +			.sa_flags = SA_RESTART,
> +			.sa_handler = tap_trigger_cb,
> +		};
> +
> +		if (sigaction(signo, &sa, NULL) == -1) {
> +			TAP_LOG(WARNING,
> +				"Unable to set rt-signal %d handler\n", signo);
> +			goto error;
> +		}
> +
> +		/* Found a good signal to use */
> +		TAP_LOG(DEBUG,
> +			"Using rt-signal %d", signo);
> +		break;
> +	}
> +
> +	if (signo == SIGRTMAX) {
> +		TAP_LOG(WARNING, "All rt-signals are in use\n");
> +
>  		/* Disable trigger globally in case of error */
>  		tap_trigger = 0;
> -		TAP_LOG(WARNING, "Rx trigger disabled: %s",
> -			strerror(errno));
> -	}
> +		TAP_LOG(NOTICE, "No Rx trigger signal available\n");
> +	} else {
> +		/* Enable signal on file descriptor */
> +		if (fcntl(fd, F_SETSIG, signo) < 0) {
> +			TAP_LOG(WARNING, "Unable to set signo %d for fd %d:
> %s",
> +				signo, fd, strerror(errno));
> +			goto error;
> +		}
> +		if (fcntl(fd, F_SETFL, flags | O_ASYNC) < 0) {
> +			TAP_LOG(WARNING, "Unable to set fcntl flags: %s",
> +				strerror(errno));
> +			goto error;
> +		}
> 
> +		if (fcntl(fd, F_SETOWN, getpid()) < 0) {
> +			TAP_LOG(WARNING, "Unable to set fcntl owner: %s",
> +				strerror(errno));
> +			goto error;
> +		}
> +	}
>  	return fd;
> 
>  error:
> --
> 2.27.0
>