| Message ID | 20200714215108.22437-1-dharmik.thakkar@arm.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id AF481A0540; Tue, 14 Jul 2020 23:51:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C85761C1FD; Tue, 14 Jul 2020 23:51:44 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by dpdk.org (Postfix) with ESMTP id 8E87B1C1F3 for <dev@dpdk.org>; Tue, 14 Jul 2020 23:51:41 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9EF9730E; Tue, 14 Jul 2020 14:51:40 -0700 (PDT) Received: from localhost.localdomain (2p2660v4-1.austin.arm.com [10.118.12.95]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 981173F66E; Tue, 14 Jul 2020 14:51:40 -0700 (PDT) From: Dharmik Thakkar <dharmik.thakkar@arm.com> To: Cc: dev@dpdk.org, nd@arm.com, Dharmik Thakkar <dharmik.thakkar@arm.com> Date: Tue, 14 Jul 2020 16:51:02 -0500 Message-Id: <20200714215108.22437-1-dharmik.thakkar@arm.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20200520032023.2649-2-dharmik.thakkar@arm.com> References: <20200520032023.2649-2-dharmik.thakkar@arm.com> Subject: [dpdk-dev] [PATCH 0/6] app/testpmd: add runtime config X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
| Series |
app/testpmd: add runtime config
|
|
Message
Dharmik Thakkar
July 14, 2020, 9:51 p.m. UTC
Meson build system lacks support for CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS configuration options. One solution is to add these options within meson_options.txt Since adding these as runtime config causes no performance impact, it makes sense to enable these as runtime config alongside other available runtime options within testpmd. Dharmik Thakkar (5): app/testpmd: add record-core-cycles runtime config doc: add record-core-cycles to testpmd funcs doc app/testpmd: add record-burst-stats runtime config doc: add record-burst-stats to testpmd funcs doc app/testpmd: enable empty polls in 5tswap Phil Yang (1): app/testpmd: enable burst stats for noisy vnf mode app/test-pmd/5tswap.c | 25 ++---- app/test-pmd/cmdline.c | 92 +++++++++++++++++++++ app/test-pmd/config.c | 12 +++ app/test-pmd/csumonly.c | 24 ++---- app/test-pmd/flowgen.c | 21 ++--- app/test-pmd/icmpecho.c | 24 ++---- app/test-pmd/iofwd.c | 26 ++---- app/test-pmd/macfwd.c | 25 ++---- app/test-pmd/macswap.c | 24 ++---- app/test-pmd/noisy_vnf.c | 4 + app/test-pmd/parameters.c | 11 ++- app/test-pmd/rxonly.c | 19 +---- app/test-pmd/testpmd.c | 77 ++++++++--------- app/test-pmd/testpmd.h | 38 +++++++-- app/test-pmd/txonly.c | 20 +---- doc/guides/testpmd_app_ug/run_app.rst | 8 ++ doc/guides/testpmd_app_ug/testpmd_funcs.rst | 30 +++++++ 17 files changed, 272 insertions(+), 208 deletions(-)
Comments
On 7/14/2020 10:51 PM, Dharmik Thakkar wrote: > Meson build system lacks support for > CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and > CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS configuration options. > > One solution is to add these options within meson_options.txt > > Since adding these as runtime config causes no performance impact, Hi Dharmik, These are on the datapath, and even disable there will be additional checks, isn't it expected to have some impact? Did you do any measurements for it? > it makes sense to enable these as runtime config alongside other > available runtime options within testpmd. If there is performance impact, another option can be still using the compile time config with meson. As Jerin suggested previously, or Ed is doing in his patch right now, something like: #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES const int record_core_cycles = 1; #else const int record_core_cycles = 0; #endif static inline void get_start_cycles(uint64_t *start_tsc) { if (record_core_cycles) *start_tsc = rte_rdtsc(); } When meson compiled by default, compiler will optimize out the code during compile time. To enable it, can provide the config option with CFLAGS, CFLAGS="-DRTE_TEST_PMD_RECORD_CORE_CYCLES" meson build > > Dharmik Thakkar (5): > app/testpmd: add record-core-cycles runtime config > doc: add record-core-cycles to testpmd funcs doc > app/testpmd: add record-burst-stats runtime config > doc: add record-burst-stats to testpmd funcs doc > app/testpmd: enable empty polls in 5tswap > > Phil Yang (1): > app/testpmd: enable burst stats for noisy vnf mode > > app/test-pmd/5tswap.c | 25 ++---- > app/test-pmd/cmdline.c | 92 +++++++++++++++++++++ > app/test-pmd/config.c | 12 +++ > app/test-pmd/csumonly.c | 24 ++---- > app/test-pmd/flowgen.c | 21 ++--- > app/test-pmd/icmpecho.c | 24 ++---- > app/test-pmd/iofwd.c | 26 ++---- > app/test-pmd/macfwd.c | 25 ++---- > app/test-pmd/macswap.c | 24 ++---- > app/test-pmd/noisy_vnf.c | 4 + > app/test-pmd/parameters.c | 11 ++- > app/test-pmd/rxonly.c | 19 +---- > app/test-pmd/testpmd.c | 77 ++++++++--------- > app/test-pmd/testpmd.h | 38 +++++++-- > app/test-pmd/txonly.c | 20 +---- > doc/guides/testpmd_app_ug/run_app.rst | 8 ++ > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 30 +++++++ > 17 files changed, 272 insertions(+), 208 deletions(-) >
On Wed, Aug 26, 2020 at 05:33:20PM +0100, Ferruh Yigit wrote: > On 7/14/2020 10:51 PM, Dharmik Thakkar wrote: > > Meson build system lacks support for > > CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and > > CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS configuration options. > > > > One solution is to add these options within meson_options.txt > > > > Since adding these as runtime config causes no performance impact, > > Hi Dharmik, > > These are on the datapath, and even disable there will be additional > checks, isn't it expected to have some impact? Did you do any > measurements for it? > Branches that always predict the same way can be very cheap, and unless proven to be a problem, I'd see no issue with having a few on the datapath - especially if it just one or two per burst. If we start seeing a significant number, or ones that occur for every packet, then we perhaps need to be more cautious. I also think that using lots of different CFLAGS for turning things on and off is as bad - or even worse - than using the old build-time config options, just this time we don't have a single list of them somewhere! Therefore, I think we really need to start converting these to runtime options where we can. Regards, /Bruce
> On Aug 26, 2020, at 11:41 AM, Bruce Richardson <bruce.richardson@intel.com> wrote: > > On Wed, Aug 26, 2020 at 05:33:20PM +0100, Ferruh Yigit wrote: >> On 7/14/2020 10:51 PM, Dharmik Thakkar wrote: >>> Meson build system lacks support for >>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and >>> CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS configuration options. >>> >>> One solution is to add these options within meson_options.txt >>> >>> Since adding these as runtime config causes no performance impact, >> >> Hi Dharmik, >> >> These are on the datapath, and even disable there will be additional >> checks, isn't it expected to have some impact? Did you do any >> measurements for it? Hi Ferruh, In my measurements, I saw a maximum performance degradation of 0.3% for rx and tx throughput (pps) with runtime option (both disabled and enabled cases) when compared to compile time option.. There was no difference in cycles per packet measurement. I did these measurements with Mellanox-ConnectX-5 card on N1SDP server with ’set fwd mac retry’ option. >> > Branches that always predict the same way can be very cheap, and unless > proven to be a problem, I'd see no issue with having a few on the datapath > - especially if it just one or two per burst. If we start seeing a > significant number, or ones that occur for every packet, then we perhaps > need to be more cautious. > > I also think that using lots of different CFLAGS for turning things on and > off is as bad - or even worse - than using the old build-time config > options, just this time we don't have a single list of them somewhere! > Therefore, I think we really need to start converting these to runtime > options where we can. > > Regards, > /Bruce
On 8/26/2020 5:41 PM, Bruce Richardson wrote: > On Wed, Aug 26, 2020 at 05:33:20PM +0100, Ferruh Yigit wrote: >> On 7/14/2020 10:51 PM, Dharmik Thakkar wrote: >>> Meson build system lacks support for >>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and >>> CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS configuration options. >>> >>> One solution is to add these options within meson_options.txt >>> >>> Since adding these as runtime config causes no performance impact, >> >> Hi Dharmik, >> >> These are on the datapath, and even disable there will be additional >> checks, isn't it expected to have some impact? Did you do any >> measurements for it? >> > Branches that always predict the same way can be very cheap, and unless > proven to be a problem, I'd see no issue with having a few on the datapath > - especially if it just one or two per burst. If we start seeing a > significant number, or ones that occur for every packet, then we perhaps > need to be more cautious. > > I also think that using lots of different CFLAGS for turning things on and > off is as bad - or even worse - than using the old build-time config > options, just this time we don't have a single list of them somewhere! > Therefore, I think we really need to start converting these to runtime > options where we can. +1 using config options via 'CFLAGS' is worse. But if we want it to be compile time option because of performance, this is an option.
On 8/26/2020 6:07 PM, Dharmik Thakkar wrote: > > >> On Aug 26, 2020, at 11:41 AM, Bruce Richardson <bruce.richardson@intel.com> wrote: >> >> On Wed, Aug 26, 2020 at 05:33:20PM +0100, Ferruh Yigit wrote: >>> On 7/14/2020 10:51 PM, Dharmik Thakkar wrote: >>>> Meson build system lacks support for >>>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and >>>> CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS configuration options. >>>> >>>> One solution is to add these options within meson_options.txt >>>> >>>> Since adding these as runtime config causes no performance impact, >>> >>> Hi Dharmik, >>> >>> These are on the datapath, and even disable there will be additional >>> checks, isn't it expected to have some impact? Did you do any >>> measurements for it? > > Hi Ferruh, > > In my measurements, I saw a maximum performance degradation of 0.3% for rx and tx throughput (pps) > with runtime option (both disabled and enabled cases) when compared to compile time option.. > There was no difference in cycles per packet measurement. > I did these measurements with Mellanox-ConnectX-5 card on N1SDP server with ’set fwd mac retry’ option. > Thanks Dharmik, 0.3% does not look a lot, I also don't get any recognizable drop when the stats are disabled (there is a drop (up to %15) when stats are enabled but that is OK). And there is a benefit to have the stats runtime configurable. This minor drop for runtime configuration looks OK to me, but lets wait a little more if there is any strong objection to it, we can proceed afterwards. > >>> >> Branches that always predict the same way can be very cheap, and unless >> proven to be a problem, I'd see no issue with having a few on the datapath >> - especially if it just one or two per burst. If we start seeing a >> significant number, or ones that occur for every packet, then we perhaps >> need to be more cautious. >> >> I also think that using lots of different CFLAGS for turning things on and >> off is as bad - or even worse - than using the old build-time config >> options, just this time we don't have a single list of them somewhere! >> Therefore, I think we really need to start converting these to runtime >> options where we can. >> >> Regards, >> /Bruce > > >
On 8/26/2020 11:06 PM, Ferruh Yigit wrote: > On 8/26/2020 6:07 PM, Dharmik Thakkar wrote: >> >> >>> On Aug 26, 2020, at 11:41 AM, Bruce Richardson <bruce.richardson@intel.com> wrote: >>> >>> On Wed, Aug 26, 2020 at 05:33:20PM +0100, Ferruh Yigit wrote: >>>> On 7/14/2020 10:51 PM, Dharmik Thakkar wrote: >>>>> Meson build system lacks support for >>>>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and >>>>> CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS configuration options. >>>>> >>>>> One solution is to add these options within meson_options.txt >>>>> >>>>> Since adding these as runtime config causes no performance impact, >>>> >>>> Hi Dharmik, >>>> >>>> These are on the datapath, and even disable there will be additional >>>> checks, isn't it expected to have some impact? Did you do any >>>> measurements for it? >> >> Hi Ferruh, >> >> In my measurements, I saw a maximum performance degradation of 0.3% for rx and tx throughput (pps) >> with runtime option (both disabled and enabled cases) when compared to compile time option.. >> There was no difference in cycles per packet measurement. >> I did these measurements with Mellanox-ConnectX-5 card on N1SDP server with ’set fwd mac retry’ option. >> > > Thanks Dharmik, > > 0.3% does not look a lot, I also don't get any recognizable drop when the stats > are disabled (there is a drop (up to %15) when stats are enabled but that is OK). > And there is a benefit to have the stats runtime configurable. > > This minor drop for runtime configuration looks OK to me, but lets wait a little > more if there is any strong objection to it, we can proceed afterwards. There seems no objection to switching runtime configuration. For series, Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com> Series applied to dpdk-next-net/main, thanks. (doc patches merged to the code while merging.)