From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 78033A0548; Wed, 21 Apr 2021 09:56:41 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3A6DF4199D; Wed, 21 Apr 2021 09:56:41 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id AF7974195D for ; Wed, 21 Apr 2021 09:56:39 +0200 (CEST) IronPort-SDR: IbNwq2Yq0sATfx8fxgQQ7+OCtfgsM91KyxUpWIGt/x6daOI4Q5nKdlhoXY4BHml5B4DfvjxyoO Mnqx+u0vwXFw== X-IronPort-AV: E=McAfee;i="6200,9189,9960"; a="183146216" X-IronPort-AV: E=Sophos;i="5.82,238,1613462400"; d="scan'208";a="183146216" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2021 00:56:37 -0700 IronPort-SDR: uhLgZiONIcznM16JwCpTnxVlLR9PdmLTsCigUiAURdPj2+ahZf7V7L3jfI1WANNVVhw6b3yw0J HkucbXNGX0wg== X-IronPort-AV: E=Sophos;i="5.82,238,1613462400"; d="scan'208";a="455235483" Received: from yvankina-mobl.ger.corp.intel.com (HELO [10.213.206.218]) ([10.213.206.218]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2021 00:56:35 -0700 To: Huisong Li , Kevin Traynor Cc: xiaoyun.li@intel.com, "Min Hu (Connor)" , dev@dpdk.org References: <1618469214-35316-1-git-send-email-humin29@huawei.com> <08a3555c-cc5f-5ba0-3d8f-cf5270703ade@huawei.com> From: Ferruh Yigit X-User: ferruhy Message-ID: Date: Wed, 21 Apr 2021 08:56:31 +0100 MIME-Version: 1.0 In-Reply-To: <08a3555c-cc5f-5ba0-3d8f-cf5270703ade@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] app/testpmd: support the query of link flow ctrl info X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 4/21/2021 7:18 AM, Huisong Li wrote: > Hi Kevin, > > Thank you for your review. > > This patchset has been applied to dpdk-next-net/main. I will send a new patch to > fix it. > > Your suggestion is better.  I intend to use the following format: > > ********************* Flow control info for port 0 ********************* > FC mode: >     Rx pause: off >     Tx pause: off > Autoneg: on > Pause time: 0x680 > High waterline: 0x80 > Low waterline: 0x40 > Send XON: on > Forward MAC control frames: off > *********************************** End ******************************** > I missed the outstanding comments from Kevin, I will drop the patch from next-net, we can proceed with next version. > > 在 2021/4/19 23:30, Kevin Traynor 写道: >> On 15/04/2021 07:46, Min Hu (Connor) wrote: >>> From: Huisong Li >>> >>> This patch supports the query of the link flow control parameter >>> on a port. >>> >>> The command format is as follows: >>> show port flow_ctrl >>> >>> Signed-off-by: Huisong Li >>> Signed-off-by: Min Hu (Connor) >> Hi Connor, >> >> Just a few minor comments, >> >> thanks, >> Kevin. >> >>> --- >>>   app/test-pmd/cmdline.c                      | 83 +++++++++++++++++++++++++++++ >>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++ >>>   2 files changed, 90 insertions(+) >>> >>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c >>> index 5bf1497..9fa85c4 100644 >>> --- a/app/test-pmd/cmdline.c >>> +++ b/app/test-pmd/cmdline.c >>> @@ -258,6 +258,9 @@ static void cmd_help_long_parsed(void *parsed_result, >>>               "show port (port_id) fec_mode" >>>               "    Show fec mode of a port.\n\n" >>> + >>> +            "show port flow_ctrl" >>> +            "    Show flow control info of a port.\n\n" >>>           ); >>>       } >>> @@ -6863,6 +6866,85 @@ cmdline_parse_inst_t cmd_set_allmulti_mode_one = { >>>       }, >>>   }; >>> +/* *** GET CURRENT ETHERNET LINK FLOW CONTROL *** */ >>> +struct cmd_link_flow_ctrl_show { >>> +    cmdline_fixed_string_t show; >>> +    cmdline_fixed_string_t port; >>> +    portid_t port_id; >>> +    cmdline_fixed_string_t flow_ctrl; >>> +}; >>> + >>> +cmdline_parse_token_string_t cmd_lfc_show_show = >>> +    TOKEN_STRING_INITIALIZER(struct cmd_link_flow_ctrl_show, >>> +                show, "show"); >>> +cmdline_parse_token_string_t cmd_lfc_show_port = >>> +    TOKEN_STRING_INITIALIZER(struct cmd_link_flow_ctrl_show, >>> +                port, "port"); >>> +cmdline_parse_token_num_t cmd_lfc_show_portid = >>> +    TOKEN_NUM_INITIALIZER(struct cmd_link_flow_ctrl_show, >>> +                port_id, RTE_UINT16); >>> +cmdline_parse_token_string_t cmd_lfc_show_flow_ctrl = >>> +    TOKEN_STRING_INITIALIZER(struct cmd_link_flow_ctrl_show, >>> +                flow_ctrl, "flow_ctrl"); >>> + >>> +static void >>> +cmd_link_flow_ctrl_show_parsed(void *parsed_result, >>> +                  __rte_unused struct cmdline *cl, >>> +                  __rte_unused void *data) >>> +{ >>> +    struct cmd_link_flow_ctrl_show *res = parsed_result; >>> +    static const char *info_border = "*********************"; >>> +    struct rte_eth_fc_conf fc_conf; >>> +    bool rx_fc_en = false; >>> +    bool tx_fc_en = false; >>> +    int ret; >>> + >>> +    if (!rte_eth_dev_is_valid_port(res->port_id)) { >> This ^ is already checked in rte_eth_dev_flow_ctrl_get() below. That >> said, it's not doing any harm here and you would need to add check for >> -ENODEV below if you wanted to tell the user that the port is invalid >> so either way is ok IMHO. >> >>> +        printf("Invalid port id %u\n", res->port_id); >>> +        return; >>> +    } >>> + >>> +    ret = rte_eth_dev_flow_ctrl_get(res->port_id, &fc_conf); >>> +    if (ret != 0) { >>> +        printf("Failed to get current flow ctrl information: err = %d\n", >>> +               ret); >>> +        return; >>> +    } >>> + >>> +    if ((fc_conf.mode == RTE_FC_RX_PAUSE) || (fc_conf.mode == RTE_FC_FULL)) >> You can remove unnecessary brackets, i.e. >> if (fc_conf.mode == RTE_FC_RX_PAUSE || fc_conf.mode == RTE_FC_FULL) >> >> >>> +        rx_fc_en = true; >>> +    if ((fc_conf.mode == RTE_FC_TX_PAUSE) || (fc_conf.mode == RTE_FC_FULL)) >> same comment on brackets >> >>> +        tx_fc_en = true; >>> + >>> +    printf("\n%s Flow control infos for port %-2d %s\n", >> s/infos/info/ >> >>> +        info_border, res->port_id, info_border); >>> +    printf("FC mode:\n"); >> It's better not to introduce a new acronym (even if it's a bit obvious) >> >>> +    printf("   Rx: %s\n", rx_fc_en ? "On" : "Off"); >>> +    printf("   Tx: %s\n", tx_fc_en ? "On" : "Off"); >>> +    printf("FC autoneg status: %s\n", fc_conf.autoneg != 0 ? "On" : "Off"); >>> +    printf("pause_time: 0x%x\n", fc_conf.pause_time); >>> +    printf("high_water: 0x%x\n", fc_conf.high_water); >>> +    printf("low_water: 0x%x\n", fc_conf.low_water); >>> +    printf("Send Xon: %s\n", fc_conf.send_xon ? "On" : "Off"); >>> +    printf("mac ctrl frame fwd: %s\n", >>> +        fc_conf.mac_ctrl_frame_fwd ? "On" : "Off"); >> There's a mix of struct member names and descriptions here. I think it's >> better to stick to one or the other. >> >> i.e. currently >> >> testpmd> show port 0 flow_ctrl >> >> ********************* Flow control infos for port 0  ********************* >> FC mode: >>     Rx: Off >>     Tx: Off >> FC autoneg status: On >> pause_time: 0x680 >> high_water: 0x80 >> low_water: 0x40 >> Send Xon: On >> Forward MAC control frames: Off >> >> >> Suggestion: >> >> ********************* Flow control info for port 0  ********************* >> Rx mode: off >> Tx mode: off >> Autoneg: on >> Pause time: 0x680 >> High water: 0x80 >> Low water: 0x40 >> Send XON: on >> Forward MAC control frames: off >> ***********************************   End  ******************************** >> >> As ever, display is subjective, so please take as suggestion, you/others >> may have different view. >> >> >>> +    printf("\n%s**************   End  ***********%s\n", >>> +        info_border, info_border); >>> +} >>> + >>> +cmdline_parse_inst_t cmd_link_flow_control_show = { >>> +    .f = cmd_link_flow_ctrl_show_parsed, >>> +    .data = NULL, >>> +    .help_str = "show port flow_ctrl", >>> +    .tokens = { >>> +        (void *)&cmd_lfc_show_show, >>> +        (void *)&cmd_lfc_show_port, >>> +        (void *)&cmd_lfc_show_portid, >>> +        (void *)&cmd_lfc_show_flow_ctrl, >>> +        NULL, >>> +    }, >>> +}; >>> + >>>   /* *** SETUP ETHERNET LINK FLOW CONTROL *** */ >>>   struct cmd_link_flow_ctrl_set_result { >>>       cmdline_fixed_string_t set; >>> @@ -17001,6 +17083,7 @@ cmdline_parse_ctx_t main_ctx[] = { >>>       (cmdline_parse_inst_t *)&cmd_link_flow_control_set_xon, >>>       (cmdline_parse_inst_t *)&cmd_link_flow_control_set_macfwd, >>>       (cmdline_parse_inst_t *)&cmd_link_flow_control_set_autoneg, >>> +    (cmdline_parse_inst_t *)&cmd_link_flow_control_show, >>>       (cmdline_parse_inst_t *)&cmd_priority_flow_control_set, >>>       (cmdline_parse_inst_t *)&cmd_config_dcb, >>>       (cmdline_parse_inst_t *)&cmd_read_reg, >>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>> index e3bfed5..3fca011 100644 >>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>> @@ -1526,6 +1526,13 @@ Where: >>>   * ``autoneg``: Change the auto-negotiation parameter. >>> +show flow ctrl >>> +~~~~~~~~~~~~~~ >>> + >>> +show the link flow control parameter on a port:: >>> + >>> +   testpmd> show port flow_ctrl >>> + >>>   set pfc_ctrl rx >>>   ~~~~~~~~~~~~~~~ >>> >> .