From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E2920A09EF for ; Wed, 16 Dec 2020 13:36:26 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BAC53C9C8; Wed, 16 Dec 2020 13:36:25 +0100 (CET) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 55D42C9B0; Wed, 16 Dec 2020 13:36:21 +0100 (CET) IronPort-SDR: lOa1xJdXsOYeiiOS2L7el9DZd+e6BOop3yde0JlFFEGigWLPgPMAWeSXsByvsE/Di269rexy8k ycqzvYyQ+sow== X-IronPort-AV: E=McAfee;i="6000,8403,9836"; a="154287545" X-IronPort-AV: E=Sophos;i="5.78,424,1599548400"; d="scan'208";a="154287545" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Dec 2020 04:36:19 -0800 IronPort-SDR: kSTw2OJmBCK1HSB+mFtpVBK6YDWh/p7VCt8VUdsTL4uS9ES0el7DvbL0UYCFIFdG4AscaFUhvO L0FUW4oIDcxw== X-IronPort-AV: E=Sophos;i="5.78,424,1599548400"; d="scan'208";a="369035473" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.233.132]) ([10.213.233.132]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Dec 2020 04:36:18 -0800 To: Viacheslav Ovsiienko , dev@dpdk.org Cc: thomas@monjalon.net, stable@dpdk.org References: <1607699265-5238-1-git-send-email-viacheslavo@nvidia.com> From: Ferruh Yigit Message-ID: <88be1461-4945-2c89-2e0e-42025ac330cc@intel.com> Date: Wed, 16 Dec 2020 12:36:14 +0000 MIME-Version: 1.0 In-Reply-To: <1607699265-5238-1-git-send-email-viacheslavo@nvidia.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix segment number check X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" On 12/11/2020 3:07 PM, Viacheslav Ovsiienko wrote: > The --txpkts command line parameter was silently ignored due to > application was unable to check the Tx queue ring sizes for non > configured ports [1]. > > The "set txpkts " was also rejected if there > was some stopped or /unconfigured port. > May not be for the commit log but to understand the problem here, what are the steps to reproduce the problem in "set txpkts" command? > This provides the following: > > - number of segment check is performed against > configured Tx queues only > > - the capability to send single packet is supposed to > be very basic and always supported, the setting segment > number to 1 is always allowed, no check performed > > - at the moment of Tx queue setup the descriptor number is > checked against configured segment number Not sure about this one, more comments below. > > Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments set") > Cc: stable@dpdk.org > Bugzilla ID: 584 > > Signed-off-by: Viacheslav Ovsiienko > --- > app/test-pmd/cmdline.c | 5 +++++ > app/test-pmd/config.c | 21 ++++++++++++++++----- > app/test-pmd/testpmd.c | 7 ++++++- > 3 files changed, 27 insertions(+), 6 deletions(-) > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c > index 0d2d6aa..86388a2 100644 > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -2798,6 +2798,11 @@ struct cmd_setup_rxtx_queue { > if (!numa_support || socket_id == NUMA_NO_CONFIG) > socket_id = port->socket_id; > > + if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) { > + printf("Failed to setup TX queue: " > + "not enough descriptors\n"); > + return; > + } The "port->nb_tx_desc[res->qid]" can be '0', and that is the default value in the testpmd, in that case device suggested values are used. Above check fails when '--txd' arguments is not provided. Same problem with the same check below in 'start_port()', I think both can be removed. > ret = rte_eth_tx_queue_setup(res->portid, > res->qid, > port->nb_tx_desc[res->qid], > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index b51de59..a6fccfa 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -3911,12 +3911,18 @@ struct igb_ring_desc_16_bytes { > for (queue_id = 0; queue_id < nb_txq; queue_id++) { > ret = get_tx_ring_size(port_id, queue_id, &ring_size); > > - if (ret) > + /* Do the check only for the active/configured ports. */ > + if (ret == -EINVAL) > + continue; > + if (ret) { > + printf("failed to get ring size for TX " > + "queue(%u) Port(%u) - txpkts ignored\n", > + port_id, queue_id); > return true; Is there a need to filter the '-EINVAL' errors only, the other error this function can get is '-EINVAL' & '-ENODEV' (which is 'port_id' is not valid), to simplify the logic, what do you think just ignore any kind of error and not fail in that case? > - > + } > if (ring_size < nb_segs) { > - printf("nb segments per TX packets=%u >= " > - "TX queue(%u) ring_size=%u - ignored\n", > + printf("nb segments per TX packets=%u >= TX " > + "queue(%u) ring_size=%u - txpkts ignored\n", > nb_segs, queue_id, ring_size); > return true; > } > @@ -3932,7 +3938,12 @@ struct igb_ring_desc_16_bytes { > uint16_t tx_pkt_len; > unsigned int i; > > - if (nb_segs_is_invalid(nb_segs)) > + /* > + * For single sengment settings failed check is ignored. > + * It is a very basic capability to send the single segment > + * packets, suppose it is always supported. > + */ > + if (nb_segs > 1 && nb_segs_is_invalid(nb_segs)) > return; if the user provided '--txd' in the command line, we can compare the segment size with that without going into the device configured values, as similar to what is done before: At the very beginning of the 'get_tx_ring_size()': if (nb_txd) { *ring_size = nb_txd; return 0; } So following combination of parameters will be supported "--txpkts=X,Y --txd=Z" (segment size checked against nb_txd) "--txpkts=N " (single segment, no check) "--txpkts=X,Y" (segment size not checked) And setting same in the command line always should be supported with segment size checks against the - nb_txd when '--txd=Z' provided - dynamic device provided values when '--txd=Z' not provided > > /* > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index 33fc0fd..9ea0145 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -2575,6 +2575,11 @@ struct extmem_param { > port->need_reconfig_queues = 0; > /* setup tx queues */ > for (qi = 0; qi < nb_txq; qi++) { > + if (port->nb_tx_desc[qi] < tx_pkt_nb_segs) { > + printf("Failed to setup TX queue: " > + "not enough descriptors\n"); > + goto fail; > + } Please check above comment. > if ((numa_support) && > (txring_numa[pi] != NUMA_NO_CONFIG)) > diag = rte_eth_tx_queue_setup(pi, qi, > @@ -2589,7 +2594,7 @@ struct extmem_param { > > if (diag == 0) > continue; > - > +fail: > /* Fail to setup tx queue, return */ > if (rte_atomic16_cmpset(&(port->port_status), > RTE_PORT_HANDLING, >