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 6CB62A0548; Wed, 21 Apr 2021 11:42:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 51E56419EC; Wed, 21 Apr 2021 11:42:17 +0200 (CEST) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id 8DBED419D3 for ; Wed, 21 Apr 2021 11:42:15 +0200 (CEST) Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FQFrm0HwxztWWG; Wed, 21 Apr 2021 17:39:52 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.498.0; Wed, 21 Apr 2021 17:42:06 +0800 To: Ferruh Yigit , Huisong Li , Kevin Traynor CC: , References: <1618469214-35316-1-git-send-email-humin29@huawei.com> <08a3555c-cc5f-5ba0-3d8f-cf5270703ade@huawei.com> <29c696ff-f737-12eb-4d9a-2c84ab24c1b5@intel.com> From: "Min Hu (Connor)" Message-ID: Date: Wed, 21 Apr 2021 17:42:06 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <29c696ff-f737-12eb-4d9a-2c84ab24c1b5@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected 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" 在 2021/4/21 16:05, Ferruh Yigit 写道: > On 4/21/2021 8:56 AM, Ferruh Yigit wrote: >> 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. >> > > v2 dropped from tree, patchwork status updated. Please send v3. > Hi, v3 has been sent, thanks. >>> >>> 在 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 >>>>>   ~~~~~~~~~~~~~~~ >>>>> >>>> . >> > > .