Re: [PATCH v3 1/3] eal: add lcore poll busyness telemetry

[Kevin Laatz <kevin.laatz@intel.com>]

On 26/08/2022 23:06, Mattias Rönnblom wrote:
> On 2022-08-25 17:28, Kevin Laatz wrote:
>> From: Anatoly Burakov <anatoly.burakov@intel.com>
<snip>
>>
>> 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
>
> Use an static inline function if you don't need the additional 
> expressive power of a macro.
>
> I suggest you also mention the performance implications, when this 
> function is enabled.

Keeping the performance implications of having the feature enabled in 
mind, I think the expressive power of the macro is beneficial here.


<snip>

>> diff --git a/lib/eal/common/eal_common_lcore_telemetry.c 
>> b/lib/eal/common/eal_common_lcore_telemetry.c
>> new file mode 100644
>> index 0000000000..bba0afc26d
>> --- /dev/null
>> +++ b/lib/eal/common/eal_common_lcore_telemetry.c
>> @@ -0,0 +1,293 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2010-2014 Intel Corporation
>> + */
>> +
>> +#include <unistd.h>
>> +#include <limits.h>
>> +#include <string.h>
>> +
>> +#include <rte_common.h>
>> +#include <rte_cycles.h>
>> +#include <rte_errno.h>
>> +#include <rte_lcore.h>
>> +
>> +#ifdef RTE_LCORE_POLL_BUSYNESS
>> +#include <rte_telemetry.h>
>> +#endif
>> +
>> +int __rte_lcore_telemetry_enabled;
>
> Is "telemetry" really the term to use here? Isn't this just another 
> piece of statistics? It can be used for telemetry, or in some other 
> fashion.
>
> (Use bool not int.)

Will change to bool.

Looking at this again, the telemetry naming is more accurate here since 
'__rte_lcore_telemetry_enabled' is used to enable/disable the telemetry 
endpoints.

-Kevin