From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vs1-f51.google.com (mail-vs1-f51.google.com [209.85.217.51]) by dpdk.org (Postfix) with ESMTP id C0E991B4CC for ; Thu, 21 Mar 2019 21:34:53 +0100 (CET) Received: by mail-vs1-f51.google.com with SMTP id g187so91788vsc.8 for ; Thu, 21 Mar 2019 13:34:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=swXVcBi2c5o2N7oOkCvD3cuwARAMnwJBsMLW6Rbg3Hg=; b=T9QfH6hSa1jwNUknIKh3PtLViowlW//0YlMp/K3UjYWJ0ato8fcj703bYGoaDszUBB NFNt7CLE1+rlaGPmlrWOOgTETvaZRIyUR8pEExsWwhKjHP6XNxcsmSPyO4tVLUlRW9Tv Q9E7FO5Yus/8nVdtVBNyr72wC4rDlI+84I+noVqwnTTNPVRAlXFQd+TjN+3R2YSHg5Sg KjKqrWaO6BSKC4HKI4Bsp1Tln5shZ9iQP4EfqsjHYizVUUOlrRW026t7kahXOm0Cbb88 /tXfT9kVRAlz4UmLiRCTAtVRfSog5szHpxH1Kmv46642wYJKEp+43sJeV4TVjBsRJ1lo 3Nkg== X-Gm-Message-State: APjAAAWfiohcPZUrM46/BQXTsN8s3nLagWMdOSzjouo4ymIM56OmbO6N etmU5XEhHRcn9VLqQLoGPt0D/Miumz/Jep12jqPeEg== X-Google-Smtp-Source: APXvYqzGKFSVNtzBnOT7xvRFNCFlKuo2a46bZJAGK1Tw90xJvcoZARLBm6lOt7o/SUHMWqnz3DmBZGriVL2geuMT8xo= X-Received: by 2002:a67:c78e:: with SMTP id t14mr3456835vsk.180.1553200493206; Thu, 21 Mar 2019 13:34:53 -0700 (PDT) MIME-Version: 1.0 References: <1552318522-18777-1-git-send-email-david.marchand@redhat.com> <1553076154-3907-1-git-send-email-david.marchand@redhat.com> <1553076154-3907-5-git-send-email-david.marchand@redhat.com> <08af7320-82ba-da2b-ae0f-edda94316805@intel.com> In-Reply-To: <08af7320-82ba-da2b-ae0f-edda94316805@intel.com> From: David Marchand Date: Thu, 21 Mar 2019 21:34:42 +0100 Message-ID: To: Ferruh Yigit Cc: dev , Wenzhuo Lu , Jingjing Wu , "Iremonger, Bernard" , Rami Rosen , Andrew Rybchenko Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v3 4/4] app/testpmd: display/clear forwarding stats on demand X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Mar 2019 20:34:54 -0000 On Thu, Mar 21, 2019 at 7:50 PM Ferruh Yigit wrote: > On 3/20/2019 10:02 AM, David Marchand wrote: > > Add a new "show/clear fwd stats all" command to display fwd and port > > statistics on the fly. > > > > To be able to do so, the (testpmd only) rte_port structure can't be used > > to maintain any statistics. > > Moved the stats dump parts from stop_packet_forwarding() and merge with > > fwd_port_stats_display() into fwd_stats_display(). > > fwd engine statistics are then aggregated into a local per port array. > > > > Signed-off-by: David Marchand > > <...> > > > +/* show/clear fwd engine statistics */ > > +struct fwd_result { > > + cmdline_fixed_string_t action; > > + cmdline_fixed_string_t fwd; > > + cmdline_fixed_string_t stats; > > + cmdline_fixed_string_t all; > > +}; > > + > > +cmdline_parse_token_string_t cmd_fwd_action = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, action, "show#clear"); > > +cmdline_parse_token_string_t cmd_fwd_fwd = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, fwd, "fwd"); > > +cmdline_parse_token_string_t cmd_fwd_stats = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, stats, "stats"); > > +cmdline_parse_token_string_t cmd_fwd_all = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, all, "all"); > > Do we need "all"? > Normally we have it when there is selection between specific or > all, > here only option is all. > Well, the thing is that if we add a specific later, we would have to introduce this "all". And since a lot of people are going to use this command, this would break their scripts ;-) I find it consistent with "show port stats all" as well. > > + > > +static void > > +cmd_fwd_parsed(void *parsed_result, > > + __rte_unused struct cmdline *cl, > > + __rte_unused void *data) > > +{ > > + struct fwd_result *res = parsed_result; > > + > > + if (!strcmp(res->action, "show")) > > + fwd_stats_display(); > > + else > > + fwd_stats_reset(); > > +} > > + > > +static cmdline_parse_inst_t cmd_fwdall = { > > + .f = cmd_fwd_parsed, > > + .data = NULL, > > + .help_str = "show|clear fwd stats all", > > + .tokens = { > > + (void *)&cmd_fwd_action, > > + (void *)&cmd_fwd_fwd, > > + (void *)&cmd_fwd_stats, > > + (void *)&cmd_fwd_all, > > + NULL, > > + }, > > +}; > > in 'app/test-pmd/cmdline.c', there is 'cmd_help_long_parsed()' function to > display the help output, can you please add new command information there > too? > Indeed, will do. > > + > > /* *** READ PORT REGISTER *** */ > > struct cmd_read_reg_result { > > cmdline_fixed_string_t read; > > @@ -18559,6 +18602,7 @@ struct cmd_show_tx_metadata_result { > > (cmdline_parse_inst_t *)&cmd_showqueue, > > (cmdline_parse_inst_t *)&cmd_showportall, > > (cmdline_parse_inst_t *)&cmd_showcfg, > > + (cmdline_parse_inst_t *)&cmd_fwdall, > > command name looks misleading 'fwdall', it feels like it is something doing > forwarding more than display, what about following same syntax with above > 'cmd_showportall', so 'cmd_showfwdall' or 'cmd_showfwd' based on your > answer to > drop "all"? > I would go with cmd_showfwdall. > <...> > > > +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES > > + fwd_cycles += fs->core_cycles; > > +#endif > > Ahh, it seems I missed this config option, looks useful :) > I wonder if it is documented anywhere. > Well, it does seem quite unknown. I am not sure of the accuracy, just tried it to check if it was really broken or not, seems to work. -- David Marchand From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id D7AF1A00E6 for ; Thu, 21 Mar 2019 21:34:56 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A419A1B52A; Thu, 21 Mar 2019 21:34:55 +0100 (CET) Received: from mail-vs1-f51.google.com (mail-vs1-f51.google.com [209.85.217.51]) by dpdk.org (Postfix) with ESMTP id C0E991B4CC for ; Thu, 21 Mar 2019 21:34:53 +0100 (CET) Received: by mail-vs1-f51.google.com with SMTP id g187so91788vsc.8 for ; Thu, 21 Mar 2019 13:34:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=swXVcBi2c5o2N7oOkCvD3cuwARAMnwJBsMLW6Rbg3Hg=; b=T9QfH6hSa1jwNUknIKh3PtLViowlW//0YlMp/K3UjYWJ0ato8fcj703bYGoaDszUBB NFNt7CLE1+rlaGPmlrWOOgTETvaZRIyUR8pEExsWwhKjHP6XNxcsmSPyO4tVLUlRW9Tv Q9E7FO5Yus/8nVdtVBNyr72wC4rDlI+84I+noVqwnTTNPVRAlXFQd+TjN+3R2YSHg5Sg KjKqrWaO6BSKC4HKI4Bsp1Tln5shZ9iQP4EfqsjHYizVUUOlrRW026t7kahXOm0Cbb88 /tXfT9kVRAlz4UmLiRCTAtVRfSog5szHpxH1Kmv46642wYJKEp+43sJeV4TVjBsRJ1lo 3Nkg== X-Gm-Message-State: APjAAAWfiohcPZUrM46/BQXTsN8s3nLagWMdOSzjouo4ymIM56OmbO6N etmU5XEhHRcn9VLqQLoGPt0D/Miumz/Jep12jqPeEg== X-Google-Smtp-Source: APXvYqzGKFSVNtzBnOT7xvRFNCFlKuo2a46bZJAGK1Tw90xJvcoZARLBm6lOt7o/SUHMWqnz3DmBZGriVL2geuMT8xo= X-Received: by 2002:a67:c78e:: with SMTP id t14mr3456835vsk.180.1553200493206; Thu, 21 Mar 2019 13:34:53 -0700 (PDT) MIME-Version: 1.0 References: <1552318522-18777-1-git-send-email-david.marchand@redhat.com> <1553076154-3907-1-git-send-email-david.marchand@redhat.com> <1553076154-3907-5-git-send-email-david.marchand@redhat.com> <08af7320-82ba-da2b-ae0f-edda94316805@intel.com> In-Reply-To: <08af7320-82ba-da2b-ae0f-edda94316805@intel.com> From: David Marchand Date: Thu, 21 Mar 2019 21:34:42 +0100 Message-ID: To: Ferruh Yigit Cc: dev , Wenzhuo Lu , Jingjing Wu , "Iremonger, Bernard" , Rami Rosen , Andrew Rybchenko Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v3 4/4] app/testpmd: display/clear forwarding stats on demand X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Message-ID: <20190321203442.riMSiaD7VLl7hF03ZSYYc3MRYe8eYGEqTDYOS6JxHhU@z> On Thu, Mar 21, 2019 at 7:50 PM Ferruh Yigit wrote: > On 3/20/2019 10:02 AM, David Marchand wrote: > > Add a new "show/clear fwd stats all" command to display fwd and port > > statistics on the fly. > > > > To be able to do so, the (testpmd only) rte_port structure can't be used > > to maintain any statistics. > > Moved the stats dump parts from stop_packet_forwarding() and merge with > > fwd_port_stats_display() into fwd_stats_display(). > > fwd engine statistics are then aggregated into a local per port array. > > > > Signed-off-by: David Marchand > > <...> > > > +/* show/clear fwd engine statistics */ > > +struct fwd_result { > > + cmdline_fixed_string_t action; > > + cmdline_fixed_string_t fwd; > > + cmdline_fixed_string_t stats; > > + cmdline_fixed_string_t all; > > +}; > > + > > +cmdline_parse_token_string_t cmd_fwd_action = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, action, "show#clear"); > > +cmdline_parse_token_string_t cmd_fwd_fwd = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, fwd, "fwd"); > > +cmdline_parse_token_string_t cmd_fwd_stats = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, stats, "stats"); > > +cmdline_parse_token_string_t cmd_fwd_all = > > + TOKEN_STRING_INITIALIZER(struct fwd_result, all, "all"); > > Do we need "all"? > Normally we have it when there is selection between specific or > all, > here only option is all. > Well, the thing is that if we add a specific later, we would have to introduce this "all". And since a lot of people are going to use this command, this would break their scripts ;-) I find it consistent with "show port stats all" as well. > > + > > +static void > > +cmd_fwd_parsed(void *parsed_result, > > + __rte_unused struct cmdline *cl, > > + __rte_unused void *data) > > +{ > > + struct fwd_result *res = parsed_result; > > + > > + if (!strcmp(res->action, "show")) > > + fwd_stats_display(); > > + else > > + fwd_stats_reset(); > > +} > > + > > +static cmdline_parse_inst_t cmd_fwdall = { > > + .f = cmd_fwd_parsed, > > + .data = NULL, > > + .help_str = "show|clear fwd stats all", > > + .tokens = { > > + (void *)&cmd_fwd_action, > > + (void *)&cmd_fwd_fwd, > > + (void *)&cmd_fwd_stats, > > + (void *)&cmd_fwd_all, > > + NULL, > > + }, > > +}; > > in 'app/test-pmd/cmdline.c', there is 'cmd_help_long_parsed()' function to > display the help output, can you please add new command information there > too? > Indeed, will do. > > + > > /* *** READ PORT REGISTER *** */ > > struct cmd_read_reg_result { > > cmdline_fixed_string_t read; > > @@ -18559,6 +18602,7 @@ struct cmd_show_tx_metadata_result { > > (cmdline_parse_inst_t *)&cmd_showqueue, > > (cmdline_parse_inst_t *)&cmd_showportall, > > (cmdline_parse_inst_t *)&cmd_showcfg, > > + (cmdline_parse_inst_t *)&cmd_fwdall, > > command name looks misleading 'fwdall', it feels like it is something doing > forwarding more than display, what about following same syntax with above > 'cmd_showportall', so 'cmd_showfwdall' or 'cmd_showfwd' based on your > answer to > drop "all"? > I would go with cmd_showfwdall. > <...> > > > +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES > > + fwd_cycles += fs->core_cycles; > > +#endif > > Ahh, it seems I missed this config option, looks useful :) > I wonder if it is documented anywhere. > Well, it does seem quite unknown. I am not sure of the accuracy, just tried it to check if it was really broken or not, seems to work. -- David Marchand