[dpdk-dev] [PATCH v7 3/6] lib: add bitrate statistics library

[Remy Horton]

On 17/01/2017 11:16, Van Haaren, Harry wrote:
[..]
>> +struct rte_stats_bitrates_s *
>> +rte_stats_bitrate_create(void)
>> +{
>> +	return rte_zmalloc(NULL, sizeof(struct rte_stats_bitrates_s),
>> +		RTE_CACHE_LINE_SIZE);
>> +}
>
> Is the socket relevant here? Perhaps pass socket_id to the function,
> and use rte_zmalloc_socket(). This function has no way of
> initializing bitrate structs on two different sockets, using one a
> single setup thread.

Not an issue in this case. It is expected that the thread that calls 
this creator will also be the thread that clocks the library (i.e. 
calling rte_stats_bitrate_calc()), and that one instance handles all 
ports. The memory block is not accessed outside of rte_stats_bitrate_calc().


>> +	/* The +-50 fixes integer rounding during divison */
>> +	if (delta > 0)
>> +		delta = (delta * alpha_percent + 50) / 100;
>> +	else
>> +		delta = (delta * alpha_percent - 50) / 100;
>> +	port_data->ewma_ibits += delta;
>
> The integer +50 feels a bit odd, I'm not opposed to this if it works though.

It was based on feedback regarding roundoffs when doing integer divides. 
Something about rounding to nearest integer rather than towards zero if 
I remember correctly..


>> +/**
>> + *  Bitrate statistics data structure.
>> + *  This data structure is intentionally opaque.
>> + */
>> +struct rte_stats_bitrates_s;
>
> _s question as previously highlighted.

Since this is a public structure, removed the _s prefix in this case. 
Looking over the coding guidelines I can't see anything explicitly 
against the notation, but existing structures have not used it.

..Remy