[PATCH 1/2] net/tap: remove redundant syscall on Tx

[PATCH 1/2] net/tap: remove redundant syscall on Tx

[Adrien Mazarguil]

Polling the Tx queue file descriptor before writing to it is not mandatory
since it is configured as non-blocking.

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Pascal Mazon <pascal.mazon@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index efc4426..c757a7c 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -43,7 +43,6 @@
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <unistd.h>
-#include <poll.h>
 #include <arpa/inet.h>
 #include <linux/if.h>
 #include <linux/if_tun.h>
@@ -242,7 +241,6 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
 	struct rte_mbuf *mbuf;
 	struct tx_queue *txq = queue;
-	struct pollfd pfd;
 	uint16_t num_tx = 0;
 	unsigned long num_tx_bytes = 0;
 	int i, n;
@@ -250,26 +248,18 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	if (unlikely(nb_pkts == 0))
 		return 0;
 
-	pfd.events = POLLOUT;
-	pfd.fd = txq->fd;
 	for (i = 0; i < nb_pkts; i++) {
-		n = poll(&pfd, 1, 0);
-
+		/* copy the tx frame data */
+		mbuf = bufs[num_tx];
+		n = write(txq->fd,
+			  rte_pktmbuf_mtod(mbuf, void *),
+			  rte_pktmbuf_pkt_len(mbuf));
 		if (n <= 0)
 			break;
 
-		if (pfd.revents & POLLOUT) {
-			/* copy the tx frame data */
-			mbuf = bufs[num_tx];
-			n = write(pfd.fd, rte_pktmbuf_mtod(mbuf, void*),
-				  rte_pktmbuf_pkt_len(mbuf));
-			if (n <= 0)
-				break;
-
-			num_tx++;
-			num_tx_bytes += mbuf->pkt_len;
-			rte_pktmbuf_free(mbuf);
-		}
+		num_tx++;
+		num_tx_bytes += mbuf->pkt_len;
+		rte_pktmbuf_free(mbuf);
 	}
 
 	txq->stats.opackets += num_tx;
-- 
2.1.4

[PATCH 2/2] net/tap: add Rx trigger

[Adrien Mazarguil]

This commit adds a signal-based trigger to the Rx burst function in order
to avoid unnecessary system calls while Rx queues are empty.

Triggered Rx bursts put less pressure on the kernel, free up CPU resources
for applications and result in a noticeable performance improvement when
sharing CPU threads with other PMDs.

Measuring the traffic forwarding rate between two physical devices in
testpmd (IO mode, single thread, 64B packets) before and after adding two
tap PMD instances (4 ports total) that do not process any traffic and
comparing results yields:

Without Rx trigger:

 -15% (--burst=32)
 -62% (--burst=1)

With Rx trigger:

 -0.3% (--burst=32)
 -6% (--burst=1)

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Pascal Mazon <pascal.mazon@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 59 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index c757a7c..d5d467a 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -31,6 +31,8 @@
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <rte_atomic.h>
+#include <rte_common.h>
 #include <rte_mbuf.h>
 #include <rte_ethdev.h>
 #include <rte_malloc.h>
@@ -42,6 +44,9 @@
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
+#include <errno.h>
+#include <signal.h>
+#include <stdint.h>
 #include <unistd.h>
 #include <arpa/inet.h>
 #include <linux/if.h>
@@ -72,6 +77,8 @@ static const char *valid_arguments[] = {
 
 static int tap_unit;
 
+static volatile uint32_t tap_trigger;	/* Rx trigger */
+
 static struct rte_eth_link pmd_link = {
 	.link_speed = ETH_SPEED_NUM_10G,
 	.link_duplex = ETH_LINK_FULL_DUPLEX,
@@ -89,6 +96,7 @@ struct pkt_stats {
 
 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;
 
@@ -111,6 +119,13 @@ struct pmd_internals {
 	struct tx_queue txq[RTE_PMD_TAP_MAX_QUEUES];	/* List of TX queues */
 };
 
+static void
+tap_trigger_cb(int sig __rte_unused)
+{
+	/* Valid trigger values are nonzero */
+	tap_trigger = (tap_trigger + 1) | 0x80000000;
+}
+
 /* Tun/Tap allocation routine
  *
  * name is the number of the interface to use, unless NULL to take the host
@@ -175,6 +190,43 @@ tun_alloc(struct pmd_internals *pmd, uint16_t qid)
 		goto error;
 	}
 
+	/* Set up trigger to optimize empty Rx bursts */
+	errno = 0;
+	do {
+		struct sigaction sa;
+		int flags = fcntl(fd, F_GETFL);
+
+		if (flags == -1 || sigaction(SIGIO, NULL, &sa) == -1)
+			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;
+		}
+		/* Enable SIGIO on file descriptor */
+		fcntl(fd, F_SETFL, flags | O_ASYNC);
+		fcntl(fd, F_SETOWN, getpid());
+	} while (0);
+	if (errno) {
+		/* Disable trigger globally in case of error */
+		tap_trigger = 0;
+		RTE_LOG(WARNING, PMD, "Rx trigger disabled: %s\n",
+			strerror(errno));
+	}
+
 	if (qid == 0) {
 		if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) {
 			RTE_LOG(ERR, PMD, "ioctl failed (SIOCGIFHWADDR) (%s)\n",
@@ -204,7 +256,13 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct rx_queue *rxq = queue;
 	uint16_t num_rx;
 	unsigned long num_rx_bytes = 0;
+	uint32_t trigger = tap_trigger;
 
+	if (trigger == rxq->trigger_seen)
+		return 0;
+	if (trigger)
+		rxq->trigger_seen = trigger;
+	rte_compiler_barrier();
 	for (num_rx = 0; num_rx < nb_pkts; ) {
 		/* allocate the next mbuf */
 		mbuf = rte_pktmbuf_alloc(rxq->mp);
@@ -563,6 +621,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 
 	internals->rxq[rx_queue_id].mp = mp;
+	internals->rxq[rx_queue_id].trigger_seen = 1; /* force initial burst */
 	internals->rxq[rx_queue_id].in_port = dev->data->port_id;
 
 	/* Now get the space available for data in the mbuf */
-- 
2.1.4

Re: [PATCH 1/2] net/tap: remove redundant syscall on Tx

[Wiles, Keith]

> On Mar 14, 2017, at 8:51 PM, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
> Polling the Tx queue file descriptor before writing to it is not mandatory
> since it is configured as non-blocking.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

Acked-by: Keith Wiles <keith.wiles@intel.com>

> ---
> drivers/net/tap/rte_eth_tap.c | 26 ++++++++------------------
> 1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index efc4426..c757a7c 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -43,7 +43,6 @@
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> #include <unistd.h>
> -#include <poll.h>
> #include <arpa/inet.h>
> #include <linux/if.h>
> #include <linux/if_tun.h>
> @@ -242,7 +241,6 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> {
> 	struct rte_mbuf *mbuf;
> 	struct tx_queue *txq = queue;
> -	struct pollfd pfd;
> 	uint16_t num_tx = 0;
> 	unsigned long num_tx_bytes = 0;
> 	int i, n;
> @@ -250,26 +248,18 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> 	if (unlikely(nb_pkts == 0))
> 		return 0;
> 
> -	pfd.events = POLLOUT;
> -	pfd.fd = txq->fd;
> 	for (i = 0; i < nb_pkts; i++) {
> -		n = poll(&pfd, 1, 0);
> -
> +		/* copy the tx frame data */
> +		mbuf = bufs[num_tx];
> +		n = write(txq->fd,
> +			  rte_pktmbuf_mtod(mbuf, void *),
> +			  rte_pktmbuf_pkt_len(mbuf));
> 		if (n <= 0)
> 			break;
> 
> -		if (pfd.revents & POLLOUT) {
> -			/* copy the tx frame data */
> -			mbuf = bufs[num_tx];
> -			n = write(pfd.fd, rte_pktmbuf_mtod(mbuf, void*),
> -				  rte_pktmbuf_pkt_len(mbuf));
> -			if (n <= 0)
> -				break;
> -
> -			num_tx++;
> -			num_tx_bytes += mbuf->pkt_len;
> -			rte_pktmbuf_free(mbuf);
> -		}
> +		num_tx++;
> +		num_tx_bytes += mbuf->pkt_len;
> +		rte_pktmbuf_free(mbuf);
> 	}
> 
> 	txq->stats.opackets += num_tx;
> -- 
> 2.1.4
> 

Regards,
Keith

Re: [PATCH 2/2] net/tap: add Rx trigger

[Wiles, Keith]

> On Mar 14, 2017, at 8:51 PM, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
> This commit adds a signal-based trigger to the Rx burst function in order
> to avoid unnecessary system calls while Rx queues are empty.
> 
> Triggered Rx bursts put less pressure on the kernel, free up CPU resources
> for applications and result in a noticeable performance improvement when
> sharing CPU threads with other PMDs.
> 
> Measuring the traffic forwarding rate between two physical devices in
> testpmd (IO mode, single thread, 64B packets) before and after adding two
> tap PMD instances (4 ports total) that do not process any traffic and
> comparing results yields:
> 
> Without Rx trigger:
> 
> -15% (--burst=32)
> -62% (--burst=1)
> 
> With Rx trigger:
> 
> -0.3% (--burst=32)
> -6% (--burst=1)
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

Acked-by: Keith Wiles <keith.wiles@intel.com>

> ---
> drivers/net/tap/rte_eth_tap.c | 59 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index c757a7c..d5d467a 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -31,6 +31,8 @@
>  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
> 
> +#include <rte_atomic.h>
> +#include <rte_common.h>
> #include <rte_mbuf.h>
> #include <rte_ethdev.h>
> #include <rte_malloc.h>
> @@ -42,6 +44,9 @@
> #include <sys/socket.h>
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> +#include <errno.h>
> +#include <signal.h>
> +#include <stdint.h>
> #include <unistd.h>
> #include <arpa/inet.h>
> #include <linux/if.h>
> @@ -72,6 +77,8 @@ static const char *valid_arguments[] = {
> 
> static int tap_unit;
> 
> +static volatile uint32_t tap_trigger;	/* Rx trigger */
> +
> static struct rte_eth_link pmd_link = {
> 	.link_speed = ETH_SPEED_NUM_10G,
> 	.link_duplex = ETH_LINK_FULL_DUPLEX,
> @@ -89,6 +96,7 @@ struct pkt_stats {
> 
> 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;
> 
> @@ -111,6 +119,13 @@ struct pmd_internals {
> 	struct tx_queue txq[RTE_PMD_TAP_MAX_QUEUES];	/* List of TX queues */
> };
> 
> +static void
> +tap_trigger_cb(int sig __rte_unused)
> +{
> +	/* Valid trigger values are nonzero */
> +	tap_trigger = (tap_trigger + 1) | 0x80000000;
> +}
> +
> /* Tun/Tap allocation routine
>  *
>  * name is the number of the interface to use, unless NULL to take the host
> @@ -175,6 +190,43 @@ tun_alloc(struct pmd_internals *pmd, uint16_t qid)
> 		goto error;
> 	}
> 
> +	/* Set up trigger to optimize empty Rx bursts */
> +	errno = 0;
> +	do {
> +		struct sigaction sa;
> +		int flags = fcntl(fd, F_GETFL);
> +
> +		if (flags == -1 || sigaction(SIGIO, NULL, &sa) == -1)
> +			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;
> +		}
> +		/* Enable SIGIO on file descriptor */
> +		fcntl(fd, F_SETFL, flags | O_ASYNC);
> +		fcntl(fd, F_SETOWN, getpid());
> +	} while (0);
> +	if (errno) {
> +		/* Disable trigger globally in case of error */
> +		tap_trigger = 0;
> +		RTE_LOG(WARNING, PMD, "Rx trigger disabled: %s\n",
> +			strerror(errno));
> +	}
> +
> 	if (qid == 0) {
> 		if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) {
> 			RTE_LOG(ERR, PMD, "ioctl failed (SIOCGIFHWADDR) (%s)\n",
> @@ -204,7 +256,13 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> 	struct rx_queue *rxq = queue;
> 	uint16_t num_rx;
> 	unsigned long num_rx_bytes = 0;
> +	uint32_t trigger = tap_trigger;
> 
> +	if (trigger == rxq->trigger_seen)
> +		return 0;
> +	if (trigger)
> +		rxq->trigger_seen = trigger;
> +	rte_compiler_barrier();
> 	for (num_rx = 0; num_rx < nb_pkts; ) {
> 		/* allocate the next mbuf */
> 		mbuf = rte_pktmbuf_alloc(rxq->mp);
> @@ -563,6 +621,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
> 	}
> 
> 	internals->rxq[rx_queue_id].mp = mp;
> +	internals->rxq[rx_queue_id].trigger_seen = 1; /* force initial burst */
> 	internals->rxq[rx_queue_id].in_port = dev->data->port_id;
> 
> 	/* Now get the space available for data in the mbuf */
> -- 
> 2.1.4
> 

Regards,
Keith

Re: [PATCH 1/2] net/tap: remove redundant syscall on Tx

[Ferruh Yigit]

On 3/14/2017 1:52 PM, Wiles, Keith wrote:
> 
>> On Mar 14, 2017, at 8:51 PM, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
>>
>> Polling the Tx queue file descriptor before writing to it is not mandatory
>> since it is configured as non-blocking.
>>
>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>
> 
> Acked-by: Keith Wiles <keith.wiles@intel.com>

Series applied to dpdk-next-net/master, thanks.