Re: [PATCH v6 5/6] app/testpmd: enable TCP/IPv4, VxLAN and GRE GSO

["Kavanagh, Mark B" <mark.b.kavanagh@intel.com>]

>-----Original Message-----
>From: Ananyev, Konstantin
>Sent: Wednesday, October 4, 2017 4:09 PM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@dpdk.org
>Cc: Hu, Jiayu <jiayu.hu@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>;
>Yigit, Ferruh <ferruh.yigit@intel.com>; thomas@monjalon.net
>Subject: RE: [PATCH v6 5/6] app/testpmd: enable TCP/IPv4, VxLAN and GRE GSO
>
>
>
>> -----Original Message-----
>> From: Kavanagh, Mark B
>> Sent: Monday, October 2, 2017 5:46 PM
>> To: dev@dpdk.org
>> Cc: Hu, Jiayu <jiayu.hu@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>;
>Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit,
>> Ferruh <ferruh.yigit@intel.com>; thomas@monjalon.net; Kavanagh, Mark B
><mark.b.kavanagh@intel.com>
>> Subject: [PATCH v6 5/6] app/testpmd: enable TCP/IPv4, VxLAN and GRE GSO
>>
>> From: Jiayu Hu <jiayu.hu@intel.com>
>>
>> This patch adds GSO support to the csum forwarding engine. Oversized
>> packets transmitted over a GSO-enabled port will undergo segmentation
>> (with the exception of packet-types unsupported by the GSO library).
>> GSO support is disabled by default.
>>
>> GSO support may be toggled on a per-port basis, using the command:
>>
>>         "set port <port_id> gso on|off"
>>
>> The maximum packet length (including the packet header and payload) for
>> GSO segments may be set with the command:
>>
>>         "set gso segsz <length>"
>>
>> Show GSO configuration for a given port with the command:
>>
>> 	"show port <port_id> gso"
>>
>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> ---
>>  app/test-pmd/cmdline.c                      | 178
>++++++++++++++++++++++++++++
>>  app/test-pmd/config.c                       |  24 ++++
>>  app/test-pmd/csumonly.c                     |  69 ++++++++++-
>>  app/test-pmd/testpmd.c                      |  13 ++
>>  app/test-pmd/testpmd.h                      |  10 ++
>>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  46 +++++++
>>  6 files changed, 335 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index ccdf239..05b0ce8 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -431,6 +431,17 @@ static void cmd_help_long_parsed(void *parsed_result,
>>  			"    Set max flow number and max packet number per-flow"
>>  			" for GRO.\n\n"
>>
>> +			"set port (port_id) gso (on|off)"
>> +			"    Enable or disable Generic Segmentation Offload in"
>> +			" csum forwarding engine.\n\n"
>> +
>> +			"set gso segsz (length)\n"
>> +			"    Set max packet length for output GSO segments,"
>> +			" including packet header and payload.\n\n"
>
>Probably a  good future improvement would be to allow user to specify gso_type
>too.

Would you like to see that change implemented in time for the 17.11 release?

>
>> +
>> +			"show port (port_id) gso\n"
>> +			"    Show GSO configuration.\n\n"
>> +
>>  			"set fwd (%s)\n"
>>  			"    Set packet forwarding mode.\n\n"
>>
>> @@ -3967,6 +3978,170 @@ struct cmd_gro_set_result {
>>  	},
>>  };
>>
>> +/* *** ENABLE/DISABLE GSO *** */
>> +struct cmd_gso_enable_result {
>> +	cmdline_fixed_string_t cmd_set;
>> +	cmdline_fixed_string_t cmd_port;
>> +	cmdline_fixed_string_t cmd_keyword;
>> +	cmdline_fixed_string_t cmd_mode;
>> +	uint8_t cmd_pid;
>> +};
>> +
>> +static void
>> +cmd_gso_enable_parsed(void *parsed_result,
>> +		__attribute__((unused)) struct cmdline *cl,
>> +		__attribute__((unused)) void *data)
>> +{
>> +	struct cmd_gso_enable_result *res;
>> +
>> +	res = parsed_result;
>> +	if (!strcmp(res->cmd_keyword, "gso"))
>> +		setup_gso(res->cmd_mode, res->cmd_pid);
>> +}
>> +
>> +cmdline_parse_token_string_t cmd_gso_enable_set =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_enable_result,
>> +			cmd_set, "set");
>> +cmdline_parse_token_string_t cmd_gso_enable_port =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_enable_result,
>> +			cmd_port, "port");
>> +cmdline_parse_token_string_t cmd_gso_enable_keyword =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_enable_result,
>> +			cmd_keyword, "gso");
>> +cmdline_parse_token_string_t cmd_gso_enable_mode =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_enable_result,
>> +			cmd_mode, "on#off");
>> +cmdline_parse_token_num_t cmd_gso_enable_pid =
>> +	TOKEN_NUM_INITIALIZER(struct cmd_gso_enable_result,
>> +			cmd_pid, UINT8);
>> +
>> +cmdline_parse_inst_t cmd_gso_enable = {
>> +	.f = cmd_gso_enable_parsed,
>> +	.data = NULL,
>> +	.help_str = "set port <port_id> gso on|off",
>> +	.tokens = {
>> +		(void *)&cmd_gso_enable_set,
>> +		(void *)&cmd_gso_enable_port,
>> +		(void *)&cmd_gso_enable_pid,
>> +		(void *)&cmd_gso_enable_keyword,
>> +		(void *)&cmd_gso_enable_mode,
>> +		NULL,
>> +	},
>> +};
>> +
>> +/* *** SET MAX PACKET LENGTH FOR GSO SEGMENTS *** */
>> +struct cmd_gso_size_result {
>> +	cmdline_fixed_string_t cmd_set;
>> +	cmdline_fixed_string_t cmd_keyword;
>> +	cmdline_fixed_string_t cmd_segsz;
>> +	uint16_t cmd_size;
>> +};
>> +
>> +static void
>> +cmd_gso_size_parsed(void *parsed_result,
>> +		       __attribute__((unused)) struct cmdline *cl,
>> +		       __attribute__((unused)) void *data)
>> +{
>> +	struct cmd_gso_size_result *res = parsed_result;
>> +
>> +	if (test_done == 0) {
>> +		printf("Before setting GSO segsz, please first stop fowarding\n");
>> +		return;
>> +	}
>> +
>> +	if (!strcmp(res->cmd_keyword, "gso") &&
>> +			!strcmp(res->cmd_segsz, "segsz")) {
>> +		if (res->cmd_size == 0) {
>
>As your gso_size includes packet header too, you probably shouldn't allow
>gso_size less
>then some minimal value of l2_len + l3_len + l4_len + ...
>Another alternative change gso_ctx.gso_size to count only payload size.

Okay - I'll take a look at this, thanks.

>
>> +			printf("gso_size should be larger than 0."
>> +					" Please input a legal value\n");
>> +		} else
>> +			gso_max_segment_size = res->cmd_size;
>> +	}
>> +}
>> +
>> +cmdline_parse_token_string_t cmd_gso_size_set =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_size_result,
>> +				cmd_set, "set");
>> +cmdline_parse_token_string_t cmd_gso_size_keyword =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_size_result,
>> +				cmd_keyword, "gso");
>> +cmdline_parse_token_string_t cmd_gso_size_segsz =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_size_result,
>> +				cmd_segsz, "segsz");
>> +cmdline_parse_token_num_t cmd_gso_size_size =
>> +	TOKEN_NUM_INITIALIZER(struct cmd_gso_size_result,
>> +				cmd_size, UINT16);
>> +
>> +cmdline_parse_inst_t cmd_gso_size = {
>> +	.f = cmd_gso_size_parsed,
>> +	.data = NULL,
>> +	.help_str = "set gso segsz <length>",
>> +	.tokens = {
>> +		(void *)&cmd_gso_size_set,
>> +		(void *)&cmd_gso_size_keyword,
>> +		(void *)&cmd_gso_size_segsz,
>> +		(void *)&cmd_gso_size_size,
>> +		NULL,
>> +	},
>> +};
>> +
>> +/* *** SHOW GSO CONFIGURATION *** */
>> +struct cmd_gso_show_result {
>> +	cmdline_fixed_string_t cmd_show;
>> +	cmdline_fixed_string_t cmd_port;
>> +	cmdline_fixed_string_t cmd_keyword;
>> +	uint8_t cmd_pid;
>> +};
>> +
>> +static void
>> +cmd_gso_show_parsed(void *parsed_result,
>> +		       __attribute__((unused)) struct cmdline *cl,
>> +		       __attribute__((unused)) void *data)
>> +{
>> +	struct cmd_gso_show_result *res = parsed_result;
>> +
>> +	if (!rte_eth_dev_is_valid_port(res->cmd_pid)) {
>> +		printf("invalid port id %u\n", res->cmd_pid);
>> +		return;
>> +	}
>> +	if (!strcmp(res->cmd_keyword, "gso")) {
>> +		if (gso_ports[res->cmd_pid].enable) {
>> +			printf("Max GSO'd packet size: %uB\n"
>> +					"Supported GSO types: TCP/IPv4, "
>> +					"VxLAN with inner TCP/IPv4 packet, "
>> +					"GRE with inner TCP/IPv4  packet\n",
>> +					gso_max_segment_size);
>> +		} else
>> +			printf("GSO is not enabled on Port %u\n", res->cmd_pid);
>> +	}
>> +}
>> +
>> +cmdline_parse_token_string_t cmd_gso_show_show =
>> +TOKEN_STRING_INITIALIZER(struct cmd_gso_show_result,
>> +		cmd_show, "show");
>> +cmdline_parse_token_string_t cmd_gso_show_port =
>> +TOKEN_STRING_INITIALIZER(struct cmd_gso_show_result,
>> +		cmd_port, "port");
>> +cmdline_parse_token_string_t cmd_gso_show_keyword =
>> +	TOKEN_STRING_INITIALIZER(struct cmd_gso_show_result,
>> +				cmd_keyword, "gso");
>> +cmdline_parse_token_num_t cmd_gso_show_pid =
>> +	TOKEN_NUM_INITIALIZER(struct cmd_gso_show_result,
>> +				cmd_pid, UINT8);
>> +
>> +cmdline_parse_inst_t cmd_gso_show = {
>> +	.f = cmd_gso_show_parsed,
>> +	.data = NULL,
>> +	.help_str = "show port <port_id> gso",
>> +	.tokens = {
>> +		(void *)&cmd_gso_show_show,
>> +		(void *)&cmd_gso_show_port,
>> +		(void *)&cmd_gso_show_pid,
>> +		(void *)&cmd_gso_show_keyword,
>> +		NULL,
>> +	},
>> +};
>> +
>>  /* *** ENABLE/DISABLE FLUSH ON RX STREAMS *** */
>>  struct cmd_set_flush_rx {
>>  	cmdline_fixed_string_t set;
>> @@ -14255,6 +14430,9 @@ struct cmd_cmdfile_result {
>>  	(cmdline_parse_inst_t *)&cmd_tunnel_tso_show,
>>  	(cmdline_parse_inst_t *)&cmd_enable_gro,
>>  	(cmdline_parse_inst_t *)&cmd_gro_set,
>> +	(cmdline_parse_inst_t *)&cmd_gso_enable,
>> +	(cmdline_parse_inst_t *)&cmd_gso_size,
>> +	(cmdline_parse_inst_t *)&cmd_gso_show,
>>  	(cmdline_parse_inst_t *)&cmd_link_flow_control_set,
>>  	(cmdline_parse_inst_t *)&cmd_link_flow_control_set_rx,
>>  	(cmdline_parse_inst_t *)&cmd_link_flow_control_set_tx,
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 3ae3e1c..88d09d0 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2454,6 +2454,30 @@ struct igb_ring_desc_16_bytes {
>>  	}
>>  }
>>
>> +void
>> +setup_gso(const char *mode, uint8_t port_id)
>> +{
>> +	if (!rte_eth_dev_is_valid_port(port_id)) {
>> +		printf("invalid port id %u\n", port_id);
>> +		return;
>> +	}
>> +	if (strcmp(mode, "on") == 0) {
>> +		if (test_done == 0) {
>> +			printf("before enabling GSO,"
>> +					" please stop forwarding first\n");
>> +			return;
>> +		}
>> +		gso_ports[port_id].enable = 1;
>> +	} else if (strcmp(mode, "off") == 0) {
>> +		if (test_done == 0) {
>> +			printf("before disabling GSO,"
>> +					" please stop forwarding first\n");
>> +			return;
>> +		}
>> +		gso_ports[port_id].enable = 0;
>> +	}
>> +}
>> +
>>  char*
>>  list_pkt_forwarding_modes(void)
>>  {
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 90c8119..bd1a287 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -70,6 +70,8 @@
>>  #include <rte_string_fns.h>
>>  #include <rte_flow.h>
>>  #include <rte_gro.h>
>> +#include <rte_gso.h>
>> +
>>  #include "testpmd.h"
>>
>>  #define IP_DEFTTL  64   /* from RFC 1340. */
>> @@ -91,6 +93,7 @@
>>  /* structure that caches offload info for the current packet */
>>  struct testpmd_offload_info {
>>  	uint16_t ethertype;
>> +	uint8_t gso_enable;
>>  	uint16_t l2_len;
>>  	uint16_t l3_len;
>>  	uint16_t l4_len;
>> @@ -381,6 +384,8 @@ struct simple_gre_hdr {
>>  				get_udptcp_checksum(l3_hdr, tcp_hdr,
>>  					info->ethertype);
>>  		}
>> +		if (info->gso_enable)
>> +			ol_flags |= PKT_TX_TCP_SEG;
>>  	} else if (info->l4_proto == IPPROTO_SCTP) {
>>  		sctp_hdr = (struct sctp_hdr *)((char *)l3_hdr + info->l3_len);
>>  		sctp_hdr->cksum = 0;
>> @@ -627,6 +632,9 @@ struct simple_gre_hdr {
>>  pkt_burst_checksum_forward(struct fwd_stream *fs)
>>  {
>>  	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>> +	struct rte_mbuf *gso_segments[GSO_MAX_PKT_BURST];
>> +	struct rte_gso_ctx *gso_ctx;
>> +	struct rte_mbuf **tx_pkts_burst;
>>  	struct rte_port *txp;
>>  	struct rte_mbuf *m, *p;
>>  	struct ether_hdr *eth_hdr;
>> @@ -634,13 +642,15 @@ struct simple_gre_hdr {
>>  	uint16_t nb_rx;
>>  	uint16_t nb_tx;
>>  	uint16_t nb_prep;
>> -	uint16_t i;
>> +	uint16_t i, j;
>>  	uint64_t rx_ol_flags, tx_ol_flags;
>>  	uint16_t testpmd_ol_flags;
>>  	uint32_t retry;
>>  	uint32_t rx_bad_ip_csum;
>>  	uint32_t rx_bad_l4_csum;
>>  	struct testpmd_offload_info info;
>> +	uint16_t nb_segments = 0;
>> +	int ret;
>>
>>  #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
>>  	uint64_t start_tsc;
>> @@ -674,6 +684,8 @@ struct simple_gre_hdr {
>>  	memset(&info, 0, sizeof(info));
>>  	info.tso_segsz = txp->tso_segsz;
>>  	info.tunnel_tso_segsz = txp->tunnel_tso_segsz;
>> +	if (gso_ports[fs->tx_port].enable)
>> +		info.gso_enable = 1;
>>
>>  	for (i = 0; i < nb_rx; i++) {
>>  		if (likely(i < nb_rx - 1))
>> @@ -851,13 +863,59 @@ struct simple_gre_hdr {
>>  		}
>>  	}
>>
>> +	if (gso_ports[fs->tx_port].enable == 0)
>> +		tx_pkts_burst = pkts_burst;
>> +	else {
>> +		gso_ctx = &(current_fwd_lcore()->gso_ctx);
>> +		gso_ctx->gso_size = gso_max_segment_size;
>> +		for (i = 0; i < nb_rx; i++) {
>
>It seems quite a lot of code to handle an error case, which I suppose
>will happen pretty rare.
>Why not just:
>
>ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>					&gso_segments[nb_segments],
>					RTE_DIM(gso_segments) - nb_segments);
>If (ret < 0)  {
>    RTE_LOG(DEBUG, ....);
>     rte_free(pkts_burst[i]);
>} else
>   nb_segments += ret;

Ditto - thanks!

>
>
>
>> +			if (unlikely(nb_rx - i >= GSO_MAX_PKT_BURST -
>> +						nb_segments)) {
>> +				/*
>> +				 * insufficient space in gso_segments,
>> +				 * stop GSO.
>> +				 */
>> +				for (j = i; j < GSO_MAX_PKT_BURST -
>> +						nb_segments; j++) {
>> +					pkts_burst[j]->ol_flags &=
>> +						(~PKT_TX_TCP_SEG);
>> +					gso_segments[nb_segments++] =
>> +						pkts_burst[j];
>> +				}
>> +				for (; j < nb_rx; j++)
>> +					rte_pktmbuf_free(pkts_burst[j]);
>> +				break;
>> +			}
>> +			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>> +					&gso_segments[nb_segments],
>> +					GSO_MAX_PKT_BURST - nb_segments);
>> +			if (ret >= 1)
>> +				nb_segments += ret;
>> +			else if (ret < 0) {
>> +				/*
>> +				 * insufficient MBUFs or space in
>> +				 * gso_segments, stop GSO.
>> +				 */
>> +				for (j = i; j < nb_rx; j++) {
>> +					pkts_burst[j]->ol_flags &=
>> +						(~PKT_TX_TCP_SEG);
>> +					gso_segments[nb_segments++] =
>> +						pkts_burst[j];
>> +				}
>> +				break;
>> +			}
>> +		}
>> +		tx_pkts_burst = gso_segments;
>> +		nb_rx = nb_segments;
>> +	}
>> +
>>  	nb_prep = rte_eth_tx_prepare(fs->tx_port, fs->tx_queue,
>> -			pkts_burst, nb_rx);
>> +			tx_pkts_burst, nb_rx);
>>  	if (nb_prep != nb_rx)
>>  		printf("Preparing packet burst to transmit failed: %s\n",
>>  				rte_strerror(rte_errno));
>>
>> -	nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst,
>> +	nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, tx_pkts_burst,
>>  			nb_prep);
>>
>>  	/*
>> @@ -868,7 +926,7 @@ struct simple_gre_hdr {
>>  		while (nb_tx < nb_rx && retry++ < burst_tx_retry_num) {
>>  			rte_delay_us(burst_tx_delay_time);
>>  			nb_tx += rte_eth_tx_burst(fs->tx_port, fs->tx_queue,
>> -					&pkts_burst[nb_tx], nb_rx - nb_tx);
>> +					&tx_pkts_burst[nb_tx], nb_rx - nb_tx);
>>  		}
>>  	}
>>  	fs->tx_packets += nb_tx;
>> @@ -881,9 +939,10 @@ struct simple_gre_hdr {
>>  	if (unlikely(nb_tx < nb_rx)) {
>>  		fs->fwd_dropped += (nb_rx - nb_tx);
>>  		do {
>> -			rte_pktmbuf_free(pkts_burst[nb_tx]);
>> +			rte_pktmbuf_free(tx_pkts_burst[nb_tx]);
>>  		} while (++nb_tx < nb_rx);
>>  	}
>> +
>>  #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
>>  	end_tsc = rte_rdtsc();
>>  	core_cycles = (end_tsc - start_tsc);
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index e097ee0..97e349d 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -400,6 +400,9 @@ static int eth_event_callback(uint8_t port_id,
>>   */
>>  static int all_ports_started(void);
>>
>> +struct gso_status gso_ports[RTE_MAX_ETHPORTS];
>> +uint16_t gso_max_segment_size = ETHER_MAX_LEN - ETHER_CRC_LEN;
>> +
>>  /*
>>   * Helper function to check if socket is already discovered.
>>   * If yes, return positive value. If not, return zero.
>> @@ -570,6 +573,7 @@ static int eth_event_callback(uint8_t port_id,
>>  	unsigned int nb_mbuf_per_pool;
>>  	lcoreid_t  lc_id;
>>  	uint8_t port_per_socket[RTE_MAX_NUMA_NODES];
>> +	uint32_t gso_types = 0;
>>
>>  	memset(port_per_socket,0,RTE_MAX_NUMA_NODES);
>>
>> @@ -654,6 +658,8 @@ static int eth_event_callback(uint8_t port_id,
>>
>>  	init_port_config();
>>
>> +	gso_types = DEV_TX_OFFLOAD_TCP_TSO | DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
>> +		DEV_TX_OFFLOAD_GRE_TNL_TSO;
>>  	/*
>>  	 * Records which Mbuf pool to use by each logical core, if needed.
>>  	 */
>> @@ -664,6 +670,13 @@ static int eth_event_callback(uint8_t port_id,
>>  		if (mbp == NULL)
>>  			mbp = mbuf_pool_find(0);
>>  		fwd_lcores[lc_id]->mbp = mbp;
>> +		/* initialize GSO context */
>> +		fwd_lcores[lc_id]->gso_ctx.direct_pool = mbp;
>> +		fwd_lcores[lc_id]->gso_ctx.indirect_pool = mbp;
>> +		fwd_lcores[lc_id]->gso_ctx.gso_types = gso_types;
>> +		fwd_lcores[lc_id]->gso_ctx.gso_size = ETHER_MAX_LEN -
>> +			ETHER_CRC_LEN;
>> +		fwd_lcores[lc_id]->gso_ctx.ipid_flag = !RTE_GSO_IPID_FIXED;
>
>Just
>fwd_lcores[lc_id]->gso_ctx.ipid_flag = 0;
>should do here.

Agreed.

Thanks again,
Mark 


>
>Konstantin