[dpdk-dev] [PATCH 1/3] bitrate: change reg implementation to match API description

[dpdk-dev] [PATCH 1/3] bitrate: change reg implementation to match API description

[Kevin Traynor]

rte_stats_bitrate_reg() API states it returns 'Zero on success'.

However, the implementation directly returns the return of
rte_metrics_reg_names() which may be zero or positive on success,
with a positive value also indicating the index.

The user of rte_stats_bitrate_reg() should not care about the
index as it is stored in the opaque rte_stats_bitrates struct.

Change the implementation of rte_stats_bitrate_reg() to match
the API description by always returning zero on success.

Fixes: 2ad7ba9a6567 ("bitrate: add bitrate statistics library")

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 lib/bitratestats/rte_bitrate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/bitratestats/rte_bitrate.c b/lib/bitratestats/rte_bitrate.c
index 8fd9f47288..e23e38bc94 100644
--- a/lib/bitratestats/rte_bitrate.c
+++ b/lib/bitratestats/rte_bitrate.c
@@ -56,6 +56,8 @@ rte_stats_bitrate_reg(struct rte_stats_bitrates *bitrate_data)
 
 	return_value = rte_metrics_reg_names(&names[0], RTE_DIM(names));
-	if (return_value >= 0)
+	if (return_value >= 0) {
 		bitrate_data->id_stats_set = return_value;
+		return 0;
+	}
 	return return_value;
 }
-- 
2.31.1

[dpdk-dev] [PATCH 2/3] bitrate: change calc implementation to match API description

[Kevin Traynor]

rte_stats_bitrate_calc() API states it returns 'Negative value on error'.

However, the implementation will return the error code from
rte_eth_stats_get() which may be non-zero on error.

Change the implementation of rte_stats_bitrate_calc() to match
the API description by always returning a negative value on error.

Fixes: 2ad7ba9a6567 ("bitrate: add bitrate statistics library")

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 lib/bitratestats/rte_bitrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bitratestats/rte_bitrate.c b/lib/bitratestats/rte_bitrate.c
index e23e38bc94..1664e4863b 100644
--- a/lib/bitratestats/rte_bitrate.c
+++ b/lib/bitratestats/rte_bitrate.c
@@ -81,5 +81,5 @@ rte_stats_bitrate_calc(struct rte_stats_bitrates *bitrate_data,
 	ret_code = rte_eth_stats_get(port_id, &eth_stats);
 	if (ret_code != 0)
-		return ret_code;
+		return ret_code < 0 ? ret_code : -ret_code;
 
 	port_data = &bitrate_data->port_stats[port_id];
-- 
2.31.1

[dpdk-dev] [PATCH 3/3] bitrate: promote rte_stats_bitrate_free() to stable

[Kevin Traynor]

rte_stats_bitrate_free() has been in DPDK since 20.11.

Its signature is very basic as it just frees an opaque
data struct allocated in rte_stats_bitrate_create()
and returns void.

It's unlikely that such a basic signature would need to change
so might as well promote it to stable for the next major ABI.

Cc: Hemant Agrawal <hemant.agrawal@nxp.com>

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 lib/bitratestats/rte_bitrate.h | 3 ---
 lib/bitratestats/version.map   | 4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/bitratestats/rte_bitrate.h b/lib/bitratestats/rte_bitrate.h
index fcd1564ddc..e494389b95 100644
--- a/lib/bitratestats/rte_bitrate.h
+++ b/lib/bitratestats/rte_bitrate.h
@@ -8,6 +8,4 @@
 #include <stdint.h>
 
-#include <rte_compat.h>
-
 #ifdef __cplusplus
 extern "C" {
@@ -36,5 +34,4 @@ struct rte_stats_bitrates *rte_stats_bitrate_create(void);
  *   Pointer allocated by rte_stats_bitrate_create()
  */
-__rte_experimental
 void rte_stats_bitrate_free(struct rte_stats_bitrates *bitrate_data);
 
diff --git a/lib/bitratestats/version.map b/lib/bitratestats/version.map
index 152730bb4e..a14d21ebba 100644
--- a/lib/bitratestats/version.map
+++ b/lib/bitratestats/version.map
@@ -9,7 +9,7 @@ DPDK_21 {
 };
 
-EXPERIMENTAL {
+DPDK_22 {
 	global:
 
 	rte_stats_bitrate_free;
-};
+} DPDK_21;
-- 
2.31.1

Re: [dpdk-dev] [PATCH 1/3] bitrate: change reg implementation to match API description

[Thomas Monjalon]

09/07/2021 17:19, Kevin Traynor:
> rte_stats_bitrate_reg() API states it returns 'Zero on success'.
> 
> However, the implementation directly returns the return of
> rte_metrics_reg_names() which may be zero or positive on success,
> with a positive value also indicating the index.
> 
> The user of rte_stats_bitrate_reg() should not care about the
> index as it is stored in the opaque rte_stats_bitrates struct.
> 
> Change the implementation of rte_stats_bitrate_reg() to match
> the API description by always returning zero on success.
> 
> Fixes: 2ad7ba9a6567 ("bitrate: add bitrate statistics library")
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>

Does it require a deprecation notice?
At least I suggest a release note in API section.

What is the target for this series? 21.11?

Re: [dpdk-dev] [PATCH 1/3] bitrate: change reg implementation to match API description

[Kevin Traynor]

On 22/07/2021 20:46, Thomas Monjalon wrote:
> 09/07/2021 17:19, Kevin Traynor:
>> rte_stats_bitrate_reg() API states it returns 'Zero on success'.
>>
>> However, the implementation directly returns the return of
>> rte_metrics_reg_names() which may be zero or positive on success,
>> with a positive value also indicating the index.
>>
>> The user of rte_stats_bitrate_reg() should not care about the
>> index as it is stored in the opaque rte_stats_bitrates struct.
>>
>> Change the implementation of rte_stats_bitrate_reg() to match
>> the API description by always returning zero on success.
>>
>> Fixes: 2ad7ba9a6567 ("bitrate: add bitrate statistics library")
>>
>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> 
> Does it require a deprecation notice?

I'm not certain, but I don't think it does. It is fixing the
implementation so it behaves as the API is documented to.

> At least I suggest a release note in API section.
> 
> What is the target for this series? 21.11?
> 

No urgency, 21.11 is fine for this set.

> 
> 

Re: [dpdk-dev] [PATCH 1/3] bitrate: change reg implementation to match API description

[Thomas Monjalon]

22/07/2021 22:24, Kevin Traynor:
> On 22/07/2021 20:46, Thomas Monjalon wrote:
> > 09/07/2021 17:19, Kevin Traynor:
> >> rte_stats_bitrate_reg() API states it returns 'Zero on success'.
> >>
> >> However, the implementation directly returns the return of
> >> rte_metrics_reg_names() which may be zero or positive on success,
> >> with a positive value also indicating the index.
> >>
> >> The user of rte_stats_bitrate_reg() should not care about the
> >> index as it is stored in the opaque rte_stats_bitrates struct.
> >>
> >> Change the implementation of rte_stats_bitrate_reg() to match
> >> the API description by always returning zero on success.
> >>
> >> Fixes: 2ad7ba9a6567 ("bitrate: add bitrate statistics library")
> >>
> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> > 
> > Does it require a deprecation notice?
> 
> I'm not certain, but I don't think it does. It is fixing the
> implementation so it behaves as the API is documented to.
> 
> > At least I suggest a release note in API section.
> > 
> > What is the target for this series? 21.11?
> > 
> 
> No urgency, 21.11 is fine for this set.

Applied, thanks.