Hi David,
> -----Original Message-----
> From: David Marchand [mailto:david.march...@6wind.com]
> Sent: Monday, September 10, 2018 6:46 AM
> To: dev@dpdk.org
> Cc: olivier.m...@6wind.com; Lu, Wenzhuo <wenzhuo...@intel.com>; Wu,
> Jingjing <jingjing...@intel.com>; Iremonger, Bernard
> <bernard.iremon...@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.march...@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.
