[dpdk-dev] [PATCH 3/3] app/testpmd: add sanity checks on received/sent packets

Iremonger, Bernard bernard.iremonger at intel.com
Tue Sep 25 15:17:02 CEST 2018


Hi David,

> -----Original Message-----
> From: David Marchand [mailto:david.marchand at 6wind.com]
> Sent: Monday, September 10, 2018 6:46 AM
> To: dev at dpdk.org
> Cc: olivier.matz at 6wind.com; Lu, Wenzhuo <wenzhuo.lu at intel.com>; Wu,
> Jingjing <jingjing.wu at intel.com>; Iremonger, Bernard
> <bernard.iremonger at intel.com>
> Subject: [PATCH 3/3] app/testpmd: add sanity checks on received/sent
> packets
> 
> Make use of the newly introduced rte_mbuf_check() to (optionally) check all
> packets received/sent through a port.
> The idea behind this is to help to quickly identify badly formatted mbufs
> coming from the pmd on the rx side, and from the application on the tx side.
> Setting the verbose level to some > 0 value will dump all packets in the
> associated rx/tx callback to further help in the debugging.
> 
> Signed-off-by: David Marchand <david.marchand at 6wind.com>
> ---
>  app/test-pmd/cmdline.c    |  63 +++++++++++++++++++
>  app/test-pmd/config.c     |  23 +++++++
>  app/test-pmd/parameters.c |   7 +++
>  app/test-pmd/testpmd.c    | 123
> ++++++++++++++++++++++++++++++++++++++
>  app/test-pmd/testpmd.h    |   9 +++
>  5 files changed, 225 insertions(+)

There should probably be an entry in section 4.6 of the Testpmd User Guide for the 
"port config all sanity_check"  command.

> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 589121d69..1de533999 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -912,6 +912,9 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> 
>  			"port config (port_id) udp_tunnel_port add|rm
> vxlan|geneve (udp_port)\n\n"
>  			"    Add/remove UDP tunnel port for tunneling
> offload\n\n"
> +
> +			"port config all sanity_check (none|rx|tx|rx+tx)\n\n"
> +			"    Configure sanity checks\n\n"
>  		);
>  	}
> 
> @@ -17602,6 +17605,65 @@ cmdline_parse_inst_t
> cmd_config_per_queue_tx_offload = {
>  	}
>  };
> 
> +/* *** configure sanity check for all ports *** */ struct
> +cmd_config_sanity_check_all {
> +	cmdline_fixed_string_t port;
> +	cmdline_fixed_string_t keyword;
> +	cmdline_fixed_string_t all;
> +	cmdline_fixed_string_t sanity_check;
> +	cmdline_fixed_string_t mode;
> +};
> +
> +static void
> +cmd_config_sanity_check_all_parsed(void *parsed_result,
> +	__rte_unused struct cmdline *cl, __rte_unused void *data) {
> +	struct cmd_config_sanity_check_all *res = parsed_result;
> +	portid_t pid;
> +
> +	if (!all_ports_stopped()) {
> +		printf("Please stop all ports first\n");
> +		return;
> +	}
> +
> +	if (set_sanity_checks(res->mode) < 0)
> +		return;
> +
> +	RTE_ETH_FOREACH_DEV(pid)
> +		ports[pid].sanity_checks = sanity_checks;
> +
> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 0, 1); }
> +
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_port =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_all, port,
> "port");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_keyword =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all,
> keyword,
> +		"config");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_all =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all, all,
> +		"all");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_sanity_check
> =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all,
> +		sanity_check, "sanity_check");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_mode =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all,
> mode,
> +		"none#rx#tx#rx+tx");
> +
> +cmdline_parse_inst_t cmd_config_sanity_check_all = {
> +	.f = cmd_config_sanity_check_all_parsed,
> +	.data = NULL,
> +	.help_str = "port config all sanity_check none|rx|tx|rx+tx",
> +	.tokens = {
> +		(void *)&cmd_config_sanity_check_all_port,
> +		(void *)&cmd_config_sanity_check_all_keyword,
> +		(void *)&cmd_config_sanity_check_all_all,
> +		(void *)&cmd_config_sanity_check_all_sanity_check,
> +		(void *)&cmd_config_sanity_check_all_mode,
> +		NULL,
> +	},
> +};
> +
>  /*
> **********************************************************
> ********************** */
> 
>  /* list of instructions */
> @@ -17863,6 +17925,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_tx_offload_get_configuration,
>  	(cmdline_parse_inst_t *)&cmd_config_per_port_tx_offload,
>  	(cmdline_parse_inst_t *)&cmd_config_per_queue_tx_offload,
> +	(cmdline_parse_inst_t *)&cmd_config_sanity_check_all,
>  #ifdef RTE_LIBRTE_BPF
>  	(cmdline_parse_inst_t *)&cmd_operate_bpf_ld_parse,
>  	(cmdline_parse_inst_t *)&cmd_operate_bpf_unld_parse, diff --git
> a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 14ccd6864..f34327d02 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1849,6 +1849,9 @@ rxtx_config_display(void)
>  		printf("  port %d: RX queue number: %d Tx queue number:
> %d\n",
>  				(unsigned int)pid, nb_rxq, nb_txq);
> 
> +		if (ports[pid].sanity_checks & SANITY_CHECK_RX)
> +			printf("    RX sanity checks enabled\n");
> +
>  		printf("    Rx offloads=0x%"PRIx64" Tx
> offloads=0x%"PRIx64"\n",
>  				ports[pid].dev_conf.rxmode.offloads,
>  				ports[pid].dev_conf.txmode.offloads);
> @@ -1873,6 +1876,9 @@ rxtx_config_display(void)
>  				rx_conf[qid].offloads);
>  		}
> 
> +		if (ports[pid].sanity_checks & SANITY_CHECK_TX)
> +			printf("    TX sanity checks enabled\n");
> +
>  		/* per tx queue config only for first queue to be less verbose
> */
>  		for (qid = 0; qid < 1; qid++) {
>  			rc = rte_eth_tx_queue_info_get(pid, qid,
> &tx_qinfo); @@ -3827,3 +3833,20 @@
> port_queue_region_info_display(portid_t port_id, void *buf)
> 
>  	printf("\n\n");
>  }
> +
> +int
> +set_sanity_checks(const char *arg)
> +{
> +	if (!strcmp(arg, "rx")) {
> +		sanity_checks = SANITY_CHECK_RX;
> +		return 0;
> +	} else if (!strcmp(arg, "tx")) {
> +		sanity_checks = SANITY_CHECK_TX;
> +		return 0;
> +	} else if (!strcmp(arg, "rx+tx")) {
> +		sanity_checks =
> +			SANITY_CHECK_RX|SANITY_CHECK_TX;
> +		return 0;
> +	} else
> +		return -1;
> +}
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
> 962fad789..5a06dc592 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -190,6 +190,7 @@ usage(char* progname)
>  	printf("  --vxlan-gpe-port=N: UPD port of tunnel VXLAN-GPE\n");
>  	printf("  --mlockall: lock all memory\n");
>  	printf("  --no-mlockall: do not lock all memory\n");
> +	printf("  --sanity-checks <rx|tx|rx+tx>: enable rx/tx sanity checks on
> +mbuf\n");
>  }
> 
>  #ifdef RTE_LIBRTE_CMDLINE
> @@ -625,6 +626,7 @@ launch_args_parse(int argc, char** argv)
>  		{ "vxlan-gpe-port",		1, 0, 0 },
>  		{ "mlockall",			0, 0, 0 },
>  		{ "no-mlockall",		0, 0, 0 },
> +		{ "sanity-checks",		1, 0, 0 },
>  		{ 0, 0, 0, 0 },
>  	};
> 
> @@ -1147,6 +1149,11 @@ launch_args_parse(int argc, char** argv)
>  				do_mlockall = 1;
>  			if (!strcmp(lgopts[opt_idx].name, "no-mlockall"))
>  				do_mlockall = 0;
> +			if (!strcmp(lgopts[opt_idx].name, "sanity-checks")) {
> +				if (set_sanity_checks(optarg) < 0)
> +					rte_exit(EXIT_FAILURE,
> +						 "invalid sanity-checks
> argument\n");
> +			}
>  			break;
>  		case 'h':
>  			usage(argv[0]);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> ee48db2a3..a310431eb 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -210,6 +210,9 @@ uint8_t dcb_test = 0;  queueid_t nb_rxq = 1; /**<
> Number of RX queues per port. */  queueid_t nb_txq = 1; /**< Number of
> TX queues per port. */
> 
> +/* Sanity checks configuration */
> +uint8_t sanity_checks;
> +
>  /*
>   * Configurable number of RX/TX ring descriptors.
>   * Defaults are supplied by drivers via ethdev.
> @@ -769,6 +772,9 @@ init_config(void)
>  			port->tx_conf[k].offloads =
>  				port->dev_conf.txmode.offloads;
> 
> +		/* Configure sanity checks with initial value from cmdline */
> +		port->sanity_checks = sanity_checks;
> +
>  		/* set flag to initialize port/queue */
>  		port->need_reconfig = 1;
>  		port->need_reconfig_queues = 1;
> @@ -1632,6 +1638,59 @@ port_is_closed(portid_t port_id)
>  	return 1;
>  }
> 
> +static uint16_t
> +mbuf_tx_check(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
> +	      uint16_t nb_pkts, __rte_unused void *user_param) {
> +	unsigned int count = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		const char *reason;
> +
> +		if (verbose_level > 0)
> +			rte_pktmbuf_dump(stdout, pkts[i], 0);
> +
> +		if (!rte_mbuf_check(pkts[i], 1, &reason)) {
> +			pkts[count++] = pkts[i];
> +			continue;
> +		}
> +
> +		TESTPMD_LOG(ERR, "invalid tx mbuf on port %"PRIu16"
> queue %"
> +			    PRIu16": %s\n", port_id, queue, reason);
> +		rte_pktmbuf_free(pkts[i]);
> +	}
> +
> +	return count;
> +}
> +
> +static uint16_t
> +mbuf_rx_check(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
> +	      uint16_t nb_pkts, __rte_unused uint16_t max_pkts,
> +	      __rte_unused void *user_param)
> +{
> +	unsigned int count = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		const char *reason;
> +
> +		if (verbose_level > 0)
> +			rte_pktmbuf_dump(stdout, pkts[i], 0);
> +
> +		if (!rte_mbuf_check(pkts[i], 1, &reason)) {
> +			pkts[count++] = pkts[i];
> +			continue;
> +		}
> +
> +		TESTPMD_LOG(ERR, "invalid rx mbuf on port %"PRIu16"
> queue %"
> +			    PRIu16": %s\n", port_id, queue, reason);
> +		rte_pktmbuf_free(pkts[i]);
> +	}
> +
> +	return count;
> +}
> +
>  int
>  start_port(portid_t pid)
>  {
> @@ -1641,6 +1700,7 @@ start_port(portid_t pid)
>  	struct rte_port *port;
>  	struct ether_addr mac_addr;
>  	enum rte_eth_event_type event_type;
> +	struct rte_eth_dev_info dev_info;
> 
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return 0;
> @@ -1671,6 +1731,32 @@ start_port(portid_t pid)
>  				}
>  			}
> 
> +			/* Free any remaining rx/tx callbacks before changing
> +			 * rxq/txq count.
> +			 */
> +			rte_eth_dev_info_get(pi, &dev_info);
> +			for (qi = 0; qi < dev_info.nb_tx_queues; qi++) {
> +				if (!port->tx_checks_cb[qi])
> +					continue;
> +				if (rte_eth_remove_tx_callback(pi, qi,
> +						port->tx_checks_cb[qi]) < 0) {
> +					/* try to reconfigure port next time
> */
> +					port->need_reconfig = 1;
> +					return -1;
> +				}
> +				port->tx_checks_cb[qi] = NULL;
> +			}
> +			for (qi = 0; qi < dev_info.nb_rx_queues; qi++) {
> +				if (!port->rx_checks_cb[qi])
> +					continue;
> +				if (rte_eth_remove_rx_callback(pi, qi,
> +						port->rx_checks_cb[qi]) < 0) {
> +					/* try to reconfigure port next time
> */
> +					port->need_reconfig = 1;
> +					return -1;
> +				}
> +				port->rx_checks_cb[qi] = NULL;
> +			}
>  			printf("Configuring Port %d (socket %u)\n", pi,
>  					port->socket_id);
>  			/* configure port */
> @@ -1703,6 +1789,24 @@ start_port(portid_t pid)
>  						port->socket_id,
>  						&(port->tx_conf[qi]));
> 
> +				if (diag == 0 &&
> +				    port->tx_checks_cb[qi]) {
> +					if (!rte_eth_remove_tx_callback(pi,
> qi,
> +							port-
> >tx_checks_cb[qi]))
> +						port->tx_checks_cb[qi] =
> NULL;
> +					else
> +						diag = -1;
> +				}
> +
> +				if (diag == 0 &&
> +				    port->sanity_checks & SANITY_CHECK_TX)
> {
> +					port->tx_checks_cb[qi] =
> +						rte_eth_add_tx_callback(pi,
> qi,
> +							mbuf_tx_check,
> NULL);
> +					if (!port->tx_checks_cb[qi])
> +						diag = -1;
> +				}
> +
>  				if (diag == 0)
>  					continue;
> 
> @@ -1753,6 +1857,25 @@ start_port(portid_t pid)
>  					     &(port->rx_conf[qi]),
>  					     mp);
>  				}
> +
> +				if (diag == 0 &&
> +				    port->rx_checks_cb[qi]) {
> +					if (!rte_eth_remove_rx_callback(pi,
> qi,
> +							port-
> >rx_checks_cb[qi]))
> +						port->rx_checks_cb[qi] =
> NULL;
> +					else
> +						diag = -1;
> +				}
> +
> +				if (diag == 0 &&
> +				    port->sanity_checks & SANITY_CHECK_RX)
> {
> +					port->rx_checks_cb[qi] =
> +						rte_eth_add_rx_callback(pi,
> qi,
> +							mbuf_rx_check,
> NULL);
> +					if (!port->rx_checks_cb[qi])
> +						diag = -1;
> +				}
> +
>  				if (diag == 0)
>  					continue;
> 
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> a1f661472..bdab372b2 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -180,6 +180,11 @@ struct rte_port {
>  	uint32_t                mc_addr_nb; /**< nb. of addr. in mc_addr_pool */
>  	uint8_t                 slave_flag; /**< bonding slave port */
>  	struct port_flow        *flow_list; /**< Associated flows. */
> +#define SANITY_CHECK_RX ((uint8_t)1 << 0) #define SANITY_CHECK_TX
> +((uint8_t)1 << 1)
> +	uint8_t                 sanity_checks;
> +	const struct rte_eth_rxtx_callback
> *rx_checks_cb[MAX_QUEUE_ID+1];
> +	const struct rte_eth_rxtx_callback
> *tx_checks_cb[MAX_QUEUE_ID+1];
>  #ifdef SOFTNIC
>  	struct softnic_port     softport;  /**< softnic params */
>  #endif
> @@ -378,6 +383,8 @@ extern int16_t tx_rs_thresh;  extern uint8_t
> dcb_config;  extern uint8_t dcb_test;
> 
> +extern uint8_t sanity_checks;
> +
>  extern uint16_t mbuf_data_size; /**< Mbuf data space size. */  extern
> uint32_t param_total_num_mbufs;
> 
> @@ -730,6 +737,8 @@ int close_file(uint8_t *buf);
> 
>  void port_queue_region_info_display(portid_t port_id, void *buf);
> 
> +int set_sanity_checks(const char *arg);
> +
>  enum print_warning {
>  	ENABLED_WARN = 0,
>  	DISABLED_WARN
> --
> 2.17.1

Regards,

Bernard.



More information about the dev mailing list