Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/22/2020 2:22 AM, Hideyuki Yamashita wrote:
> Hello,
> 
> Thanks for your comments.
> Please see my comments inline tagged with [HY].
> 
>> 04/12/2020 08:51, Hideyuki Yamashita:
>>> In general, DPDK application consumes CPU usage because it polls
>>> incoming packets using rx_burst API in infinite loop.
>>> This makes difficult to estimate how much CPU usage is really
>>> used to send/receive packets by the DPDK application.
>>>
>>> For example, even if no incoming packets arriving, CPU usage
>>> looks nearly 100% when observed by top command.
>>>
>>> It is beneficial if developers can observe real CPU usage of the
>>> DPDK application.
>>> Such information can be exported to monitoring application like
>>> prometheus/graphana and shows CPU usage graphically.
>>>
>>> To achieve above, this patch set provides apistats functionality.
>>> apistats provides the followiing two counters for each lcore.
>>> - rx_burst_counts[RTE_MAX_LCORE]
>>> - tx_burst_counts[RTE_MAX_LCORE]
>>> Those accumulates rx_burst/tx_burst counts since the application starts.
>>
>> Please could you compare with what rte_jobstats offers?
>> http://code.dpdk.org/dpdk/latest/source/lib/librte_jobstats/rte_jobstats.h
>>
>> I feel all of this shares the same goals as librte_power work.
> 
> [HY]
> Thanks for your commetns.
> You are correct. As you well know, l3fwd-power measures "how cpu cores
> are busy".
> And in that sense, the goal of my proposal is the same with yours .
> Moreover l3fwd-power is more precise than my proposal.
> 
> Point of my proposal is
> - more easy to use
> - less code impact on application code
> 
> I think that if application developer wants to need to measure "how cpu
> cores are busy" he/she will needs to implement
> - logic similar with l3fwd-power
> or
> - use jobstats API
> 
> But it is rather heavy for existing applications.
> By using my proposal, it is "much easier" to implement.
> (But it is "rough" measurement. I think it is trade-off)
>   
> How do you think about the idea?
> 
>> [...]
>>> - basic API counting functionality(apistats) into librte_ethdev
>>
>> Could it be it be accessible via rte_telemetry?
>> http://code.dpdk.org/dpdk/latest/source/lib/librte_telemetry/rte_telemetry.h
>>
> [HY]
> OK.
> Currently, no reason not using telemetry.
> 
> I think telemetry is useful for applications which does NOT call DPDK
> API(C lang API) directly.
> My patchset provide only C API to retrieve apistats.
> But if assuming not all applications call C API, then I think it is
> reasonable to add telemetry in addition to C API for exposing stats.
> 
> Do you think "exposure via C API" is not needed?
> 

Hi Hideyuki,

With current implementation the change will cause a performance degradation for 
all users, even if they use the stats or not.

Even if the statics update wrapped with #ifdef, as suggested, the functionality 
is simple and it can be easily implemented by application using Rx/Tx callbacks, 
again as suggested, without introducing this new complexity.

And for more complex usage, the jobstats already tries to provide a generic 
library for it, or application specific needs can be implemented in application 
level as done is l3fwd-power.


Overall I agree the problem this patch is trying to solve is a valid one, but 
not sure about the current implementation, and I am marking current version as 
'rejected', please feel free to send a new version with a new approach.

Thanks,
ferruh