> From: Kevin Laatz [mailto:kevin.la...@intel.com] > Sent: Friday, 26 August 2022 17.27 > > On 26/08/2022 09:29, Morten Brørup wrote: > >> From: Jerin Jacob [mailto:jerinjac...@gmail.com] > >> Sent: Friday, 26 August 2022 10.16 > >> > >> On Fri, Aug 26, 2022 at 1:37 PM Bruce Richardson > >> <bruce.richard...@intel.com> wrote: > >>> On Fri, Aug 26, 2022 at 12:35:16PM +0530, Jerin Jacob wrote: > >>>> On Thu, Aug 25, 2022 at 8:56 PM Kevin Laatz > <kevin.la...@intel.com> > >> wrote: > >>>>> From: Anatoly Burakov <anatoly.bura...@intel.com> > >>>>> > >>>>> Currently, there is no way to measure lcore poll busyness in a > >> passive way, > >>>>> without any modifications to the application. This patch adds a > >> new EAL API > >>>>> that will be able to passively track core polling busyness. > >>>>> > >>>>> The poll busyness is calculated by relying on the fact that most > >> DPDK API's > >>>>> will poll for packets. Empty polls can be counted as "idle", > >> while > >>>>> non-empty polls can be counted as busy. To measure lcore poll > >> busyness, we > >>>>> simply call the telemetry timestamping function with the number > >> of polls a > >>>>> particular code section has processed, and count the number of > >> cycles we've > >>>>> spent processing empty bursts. The more empty bursts we > >> encounter, the less > >>>>> cycles we spend in "busy" state, and the less core poll busyness > >> will be > >>>>> reported. > >>>>> > >>>>> In order for all of the above to work without modifications to > >> the > >>>>> application, the library code needs to be instrumented with calls > >> to the > >>>>> lcore telemetry busyness timestamping function. The following > >> parts of DPDK > >>>>> are instrumented with lcore telemetry calls: > >>>>> > >>>>> - All major driver API's: > >>>>> - ethdev > >>>>> - cryptodev > >>>>> - compressdev > >>>>> - regexdev > >>>>> - bbdev > >>>>> - rawdev > >>>>> - eventdev > >>>>> - dmadev > >>>>> - Some additional libraries: > >>>>> - ring > >>>>> - distributor > >>>>> > >>>>> To avoid performance impact from having lcore telemetry support, > >> a global > >>>>> variable is exported by EAL, and a call to timestamping function > >> is wrapped > >>>>> into a macro, so that whenever telemetry is disabled, it only > >> takes one > >>>>> additional branch and no function calls are performed. It is also > >> possible > >>>>> to disable it at compile time by commenting out > >> RTE_LCORE_BUSYNESS from > >>>>> build config. > >>>>> > >>>>> This patch also adds a telemetry endpoint to report lcore poll > >> busyness, as > >>>>> well as telemetry endpoints to enable/disable lcore telemetry. A > >>>>> documentation entry has been added to the howto guides to explain > >> the usage > >>>>> of the new telemetry endpoints and API. > >>>>> > >>>>> Signed-off-by: Kevin Laatz <kevin.la...@intel.com> > >>>>> Signed-off-by: Conor Walsh <conor.wa...@intel.com> > >>>>> Signed-off-by: David Hunt <david.h...@intel.com> > >>>>> Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com> > >>>>> > >>>>> --- > >>>>> v3: > >>>>> * Fix missed renaming to poll busyness > >>>>> * Fix clang compilation > >>>>> * Fix arm compilation > >>>>> > >>>>> v2: > >>>>> * Use rte_get_tsc_hz() to adjust the telemetry period > >>>>> * Rename to reflect polling busyness vs general busyness > >>>>> * Fix segfault when calling telemetry timestamp from an > >> unregistered > >>>>> non-EAL thread. > >>>>> * Minor cleanup > >>>>> --- > >>>>> diff --git a/meson_options.txt b/meson_options.txt > >>>>> index 7c220ad68d..725b851f69 100644 > >>>>> --- a/meson_options.txt > >>>>> +++ b/meson_options.txt > >>>>> @@ -20,6 +20,8 @@ option('enable_driver_sdk', type: 'boolean', > >> value: false, description: > >>>>> 'Install headers to build drivers.') > >>>>> option('enable_kmods', type: 'boolean', value: false, > >> description: > >>>>> 'build kernel modules') > >>>>> +option('enable_lcore_poll_busyness', type: 'boolean', value: > >> true, description: > >>>>> + 'enable collection of lcore poll busyness telemetry') > >>>> IMO, All fastpath features should be opt-in. i.e default should be > >> false. > >>>> For the trace fastpath related changes, We have done the similar > >> thing > >>>> even though it cost additional one cycle for disabled trace points > >>>> > >>> We do need to consider runtime and build defaults differently, > >> though. > >>> Since this has also runtime enabling, I think having build-time > >> enabling > >>> true as default is ok, so long as the runtime enabling is false > >> (assuming > >>> no noticable overhead when the feature is disabled.) > >> I was talking about buildtime only. "enable_trace_fp" meson option > >> selected as > >> false as default. > > Agree. "enable_lcore_poll_busyness" is in the fast path, so it should > follow the design pattern of "enable_trace_fp". > > +1 to making this opt-in. However, I'd lean more towards having the > buildtime option enabled and the runtime option disabled by default. > There is no measurable impact cause by the extra branch (the check for > enabled/disabled in the macro) by disabling at runtime, and we gain the > benefit of avoiding a recompile to enable it later.
The exact same thing could be said about "enable_trace_fp"; however, the development effort was put into separating it from "enable_trace", so it could be disabled by default. Your patch is unlikely to get approved if you don't follow the "enable_trace_fp" design pattern as suggested. > > > > >> If the concern is enabling on generic distros then distro generic > >> config can opt in this > >> > >>> /Bruce > > @Kevin, are you considering a roadmap for using > RTE_LCORE_TELEMETRY_TIMESTAMP() for other purposes? Otherwise, it > should also be renamed to indicate that it is part of the "poll > busyness" telemetry. > > No further purposes are planned for this macro, I'll rename it in the > next revision. OK. Thank you. Also, there's a new discussion about EAL bloat [1]. Perhaps I'm stretching it here, but it would be nice if your library was made a separate library, instead of part of the EAL library. (Since this kind of feature is not new to the EAL, I will categorize this suggestion as "nice to have", not "must have".) [1] http://inbox.dpdk.org/dev/2594603.Isy0gbHreE@thomas/T/
