Re: [dpdk-dev] [PATCH v1] maintainers: resign all maintainerships

[Remy Horton]

On 01/03/2019 10:31, Thomas Monjalon wrote:
[..]

Applied
Remy, you are welcome to continue maintaining some of these libraries
with another email address if you plan to have time to dedicate.


I've considered reclaiming them in a personal capacity but I need to see 
how time and/or contractual commitments will play out.


..Remy

[dpdk-dev] [PATCH v1] maintainers: resign all maintainerships

[Remy Horton]

After today I will no longer be with Intel, so I am
removing my name from all related maintainer roles. 

Signed-off-by: Remy Horton 
---
 MAINTAINERS | 4 
 1 file changed, 4 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3b39bc6..f13c241 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -184,7 +184,6 @@ F: test/test/test_memory.c
 F: test/test/test_memzone.c
 
 Keep alive
-M: Remy Horton 
 F: lib/librte_eal/common/include/rte_keepalive.h
 F: lib/librte_eal/common/rte_keepalive.c
 F: examples/l2fwd-keepalive/
@@ -1206,12 +1205,10 @@ F: examples/l2fwd-jobstats/
 F: doc/guides/sample_app_ug/l2_forward_job_stats.rst
 
 Metrics
-M: Remy Horton 
 F: lib/librte_metrics/
 F: test/test/test_metrics.c
 
 Bit-rate statistics
-M: Remy Horton 
 F: lib/librte_bitratestats/
 F: test/test/test_bitratestats.c
 
@@ -1292,7 +1289,6 @@ F: doc/guides/tools/proc_info.rst
 Other Example Applications
 --
 
-M: Remy Horton 
 F: examples/ethtool/
 F: doc/guides/sample_app_ug/ethtool.rst
 
-- 
2.9.5

Re: [dpdk-dev] [PATCH v5] lib/metrics: add unregister api for metrics

[Remy Horton]

On 28/02/2019 13:28, Wan Junjie wrote:

Bitratestats test fails even without my patch.  The reason is that
metrics lib has no de-init process. Harman Kalra has submited a patch
for this.
Check https://patches.dpdk.org/patch/50451


Ah forgot about that patch. :S



As for metrics lib, the array block of metadata may be better on cache
side. Since rte_metrics_update_values is called periodicly in bitrate lib.


Quite possibly, at least for smallish datasets like bitrate.



Signed-off-by: Junjie Wan 


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v5] lib/metrics: add unregister api for metrics

[Remy Horton]

The tests for metrics themselves now all pass, but there is failure with 
bitrate statistics (bitrate stats uses metrics underneath). Latency 
statistics, which also uses metrics, seems ok though.


RTE>>bitratestats_autotest
 + --- +
 + Test Suite : BitRate Stats Unit Test Suite
port in ring setup : 0
 + --- +
 + TestCase [ 0] : test_stats_bitrate_create succeeded
 + TestCase [ 1] : test_stats_bitrate_reg failed
 + TestCase [ 2] : test_stats_bitrate_reg_invalidpointer succeeded
 + TestCase [ 3] : test_stats_bitrate_calc_invalid_bitrate_data succeeded
Invalid port_id=33
 + TestCase [ 4] : test_stats_bitrate_calc_invalid_portid_1 succeeded
Invalid port_id=65535
 + TestCase [ 5] : test_stats_bitrate_calc_invalid_portid_2 succeeded
Invalid port_id=31
 + TestCase [ 6] : test_stats_bitrate_calc_non_existing_portid succeeded
 + TestCase [ 7] : test_stats_bitrate_calc succeeded
 + --- +
 + Test Suite Summary
 + Tests Total :8
 + Tests Skipped :  0
 + Tests Executed : 8
 + Tests Unsupported:   0
 + Tests Passed :   7
 + Tests Failed :   1
 + --- +
Test Failed


Personally if I was reimplementing metrics I would be inclined to 
replace the single fixed-size metadata[] array that is carved up with 
something more dynamic, perhaps a 2d linked-list. Doing so would 
probably negate the need for a bitmap.




On 27/02/2019 17:19, Junjie Wan wrote:

From: junka 

The bitmap will help maintain the metrics. We can dynamically
add and remove metrics data. For example, after uninit latency lib,
it could remove itself from the metrics. This could make the result
from rte_metrics_get_names much more simple to display the wanted
metrics data only.

Signed-off-by: Junjie Wan 

Re: [dpdk-dev] [PATCH v4] lib/metrics: add unregister api for metrics

[Remy Horton]

This patch has checkpatch errors that will need to be fixed:

ERROR:SPACING: space prohibited before that close parenthesis ')'
#179: FILE: lib/librte_metrics/rte_metrics.c:233:
+   if (count < 1 )

ERROR:TRAILING_WHITESPACE: trailing whitespace
#242: FILE: lib/librte_metrics/rte_metrics.c:298:
+^I^Ifor (idx_name = 0; idx < stats->cnt_stats && $


If the patch is applied, two of the unit-tests for metrics break:

# ./test/build/app/test
RTE>>metrics_autotest
 + --- +
 + Test Suite : Metrics Unit Test Suite
 + --- +
 + TestCase [ 0] : test_metrics_without_init succeeded
 + TestCase [ 1] : test_metrics_reg_name_with_validname succeeded
 + TestCase [ 2] : test_metrics_reg_names succeeded
 + TestCase [ 3] : test_metrics_update_value failed
 + TestCase [ 4] : test_metrics_update_values failed
 + TestCase [ 5] : test_metrics_get_names succeeded
 + TestCase [ 6] : test_metrics_get_values succeeded
 + --- +
 + Test Suite Summary
 + Tests Total :7
 + Tests Skipped :  0
 + Tests Executed : 7
 + Tests Unsupported:   0
 + Tests Passed :   5
 + Tests Failed :   2
 + --- +


Both of these issues will need to be addressed.


On 22/02/2019 15:39, wanjunjie wrote:

From: junka 

The bitmap will help maintain the metrics. We can dynamically
add and remove metrics data. For example, after uninit latency lib,
it could remove itself from the metrics. This could make the result
from rte_metrics_get_names much more simple to display the wanted
metrics data only.

Signed-off-by: junka 
---
 lib/librte_metrics/rte_metrics.c | 182 ---
 lib/librte_metrics/rte_metrics.h |  21 +
 2 files changed, 152 insertions(+), 51 deletions(-)

Re: [dpdk-dev] [PATCH] examples/ethtool: fix 2 typos

[Remy Horton]

On 25/02/2019 19:21, Rami Rosen wrote:
[snip]

Signed-off-by: Rami Rosen 
---
 examples/ethtool/lib/rte_ethtool.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH 1/2] metrics: new API to deinitialise metrics library

[Remy Horton]

This patchset is in that grey area between new feature and bugfix so it 
might need to be CC'd to sta...@dpdk.org


On 22/02/2019 14:45, Harman Kalra wrote:


Signed-off-by: Harman Kalra 
---
 doc/guides/prog_guide/metrics_lib.rst  | 14 ++
 lib/librte_metrics/rte_metrics.c   | 20 
 lib/librte_metrics/rte_metrics.h   | 17 +
 lib/librte_metrics/rte_metrics_version.map |  6 ++
 4 files changed, 57 insertions(+)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH 2/2] test/metrics: first test case fails on continuous execution

[Remy Horton]

Did quick test run and seems fine.

On 22/02/2019 14:45, Harman Kalra wrote:


Signed-off-by: Harman Kalra 
---
 test/test/test_metrics.c | 17 +
 1 file changed, 17 insertions(+)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH] test/metrics: Fixed a negative case to pass

[Remy Horton]

Git apply choked on the patch but it applied using patch -p0 - I'm 
guessing this is a problem with patches made using diff rather than git 
format-patch. The change itself is fine though. :)


On 06/12/2018 15:01, Harman Kalra wrote:

Negative test case for passing invalid count size to
rte_metrics_update_values() will pass if count value
is any value greater than 1 because set size wrt to
key id 1 is 1 and passing 0 as no of values to be
updated will not hit the ERANGE check.

Signed-off-by: Harman Kalra 


Missing fixline:

Fixes: cd3804242901 ("test/metrics: add unit tests for metrics library")

Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH 1/4] kvargs: support list value

[Remy Horton]

On 09/10/2018 16:11, Stephen Hemminger wrote:

On Tue, 09 Oct 2018 16:31:24 +0200

[..]

This is getting complex enough that doing a real parser maybe necessary.


I thought the same thing back in April with the port representor 
patchsets. Quickly run out of things to use as delimiters otherwise.




Why not lex/yacc?


Think Yacc/Bison are vastly overcomplex and a bit of a pain to 
integrate. The sort of config grammar needed here will most likely be 
LL(1), so fairly easy to do by hand anyway.




Or better yet go to real syntax like JSON.

Re: [dpdk-dev] [PATCH 3/4] ethdev: move representor parsing functions

[Remy Horton]

On 09/10/2018 03:18, Thomas Monjalon wrote:

The functions for representor devargs parsing were static
in the file rte_ethdev.c.
In order to reuse them in the file rte_class_eth.c,
they are moved to the files ethdev_private.c/.h.

Signed-off-by: Thomas Monjalon 


There is overlap between these functions and some of the rte_kvargs_*() 
functions. Is it feasible/desirable to combine them?


..Remy

Re: [dpdk-dev] [PATCH] lib/latencystats: free up the memzone

[Remy Horton]

On 26/07/2018 17:50, Reshma Pattan wrote:

Free up the memzone allocated during the
rte_latencystats_init().

Fixes: 5cd3cac9ed ("latency: added new library for latency stats")
CC: sta...@dpdk.org
CC: remy.hor...@intel.com

Signed-off-by: Reshma Pattan 


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v1] lib/metrics: add check for invalid metric keys

[Remy Horton]

Harry,

Can you add the following to the cheat-sheet? Don't do many patches 
these days, and I'm not the only one to miss these recently..


..Remy

On 26/07/2018 19:11, Thomas Monjalon wrote:

Remy,
You are not new to DPDK. Please pay attention to details:
- subject does not start with lib/
- fixes of old commits must have Cc: sta...@dpdk.org

20/07/2018 17:31, Ferruh Yigit:

On 7/6/2018 2:21 PM, Remy Horton wrote:

This patchset adds a check to rte_metrics_update_values()
that prevents the updating of metrics when presented with
an invalid metric key. Previously, doing the latter could
result in a crash.

Fixes: 349950ddb9c5 ("metrics: add information metrics library")

Signed-off-by: Remy Horton 


Acked-by: Ferruh Yigit 


Applied, thanks

Re: [dpdk-dev] [PATCH v2] test: add unit tests for metrics library

[Remy Horton]

Tested using the following patches also applied:

http://patches.dpdk.org/patch/42097/
http://patches.dpdk.org/patch/42098/
http://patches.dpdk.org/patch/42510/
http://patches.dpdk.org/patch/42971/

Two test-cases fail, but these in themselves ought not block this patch. 
Details below.


Acked-by: Remy Horton 



+ /* Test Case to check failures when memzone init is not done */
+static int
+test_metrics_without_init(void)
+{
+   int err = 0;
+   const uint64_t  value[REG_METRIC_COUNT] = {0};
+   const char * const mnames[] = {
+   "mean_bits_in", "mean_bits_out",
+   "peak_bits_in", "peak_bits_out",
+   };
+
+   /* Failure Test: Checking for memzone initialization */
+   err = rte_metrics_reg_name(NULL);
+   TEST_ASSERT(err == -EIO, "%s, %d", __func__, __LINE__);


Returns -EINVAL instead of -EIO due to NULL check coming before memzone 
check. Suggest using non-NULL value.




+/* Test case to validate update a list of  metrics  */
+static int
+test_metrics_update_values(void)
+{

[..]

+   /* Failed Test: Invalid count size */
+   err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
+KEY, &value[0], 0);
+   TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);


Test fails, fault with library: Silent handling length of zero. Will 
send patch.

Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix missing NULL point check

[Remy Horton]

On 16/07/2018 13:47, Qi Zhang wrote:

Add missing NULL point check inside ixgbe_pf_host_uninit, or it may cause
segment fault when detaching a device.

Fixes: cf80ba6e2038 ("net/ixgbe: add support for representor ports")
Cc: sta...@dpdk.org

Signed-off-by: Qi Zhang 


Acked-by: Remy Horton 

[dpdk-dev] [PATCH v2] lib/bitratestats: add NULL sanity checks

[Remy Horton]

If rte_stats_bitrate_reg() or rte_stats_bitrate_calc() are
passed NULL as the parameter for the stats structure, the
result is a crash. Fixed by adding a sanity check that makes
sure the passed-in pointer is not NULL.

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

Signed-off-by: Remy Horton 
---
 lib/librte_bitratestats/rte_bitrate.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_bitratestats/rte_bitrate.c 
b/lib/librte_bitratestats/rte_bitrate.c
index 964e3c3..c4b28f6 100644
--- a/lib/librte_bitratestats/rte_bitrate.c
+++ b/lib/librte_bitratestats/rte_bitrate.c
@@ -47,6 +47,9 @@ rte_stats_bitrate_reg(struct rte_stats_bitrates *bitrate_data)
};
int return_value;
 
+   if (bitrate_data == NULL)
+   return -EINVAL;
+
return_value = rte_metrics_reg_names(&names[0], ARRAY_SIZE(names));
if (return_value >= 0)
bitrate_data->id_stats_set = return_value;
@@ -65,6 +68,9 @@ rte_stats_bitrate_calc(struct rte_stats_bitrates 
*bitrate_data,
const int64_t alpha_percent = 20;
uint64_t values[6];
 
+   if (bitrate_data == NULL)
+   return -EINVAL;
+
ret_code = rte_eth_stats_get(port_id, ð_stats);
if (ret_code != 0)
return ret_code;
-- 
2.9.5

Re: [dpdk-dev] [PATCH v2] test: add unit tests for bitrate library

[Remy Horton]

Patch needs rebasing:

Checking patch test/test/Makefile...
error: while searching for:

SRCS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += test_cryptodev_blockcipher.c
SRCS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += test_cryptodev.c

ifeq ($(CONFIG_RTE_COMPRESSDEV_TEST),y)
SRCS-$(CONFIG_RTE_LIBRTE_COMPRESSDEV) += test_compressdev.c

error: patch failed: test/test/Makefile:179
error: test/test/Makefile: patch does not apply

Manually fixing the above, patchset seems fine.


On 12/07/2018 10:53, Jananee Parthasarathy wrote:

Unit Test Cases for BitRate library.
Dependency patch is
"add sample functions for packet forwarding"
Patch Link is http://patches.dpdk.org/patch/42946/

Signed-off-by: Chaitanya Babu Talluri 
Reviewed-by: Reshma Pattan 


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH] net/ixgbe: fix missing NULL point check

[Remy Horton]

Patch doesn't apply to latest master, but otherwise seems fine to me.

On 02/07/2018 05:18, Qi Zhang wrote:

Add missing NULL point check in ixgbe_pf_host_uninit, or it may cause
segement fault when detach a device.

Fixes: cf80ba6e2038 ("net/ixgbe: add support for representor ports")
Cc: sta...@dpdk.org

Signed-off-by: Qi Zhang 

[dpdk-dev] [PATCH v1] lib/metrics: add check for invalid metric keys

[Remy Horton]

This patchset adds a check to rte_metrics_update_values()
that prevents the updating of metrics when presented with
an invalid metric key. Previously, doing the latter could
result in a crash.

Fixes: 349950ddb9c5 ("metrics: add information metrics library")

Signed-off-by: Remy Horton 
---
 lib/librte_metrics/rte_metrics.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index 258f058..b5638f5 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -159,6 +159,11 @@ rte_metrics_update_values(int port_id,
stats = memzone->addr;
 
rte_spinlock_lock(&stats->lock);
+
+   if (key >= stats->cnt_stats) {
+   rte_spinlock_unlock(&stats->lock);
+   return -EINVAL;
+   }
idx_metric = key;
cnt_setsize = 1;
while (idx_metric < stats->cnt_stats) {
-- 
2.9.5

Re: [dpdk-dev] [PATCH] test: add unit tests for metrics library

[Remy Horton]

'noon,

See inline comments.

On 05/07/2018 08:37, Hari kumar Vemula wrote:

Unit Testcases are added for metrics library.


[..]


+/* Test case to validate registering a list of valid  metric names */
+static int
+test_metrics_reg_names(void)
+{
+   int err = 0;
+   const char * const mnames[] = {
+   "mean_bits_in", "mean_bits_out",
+   "peak_bits_in", "peak_bits_out",
+   };
+
+
+   /* Success Test: valid array and count size */
+   err = rte_metrics_reg_names(&mnames[0], ARRAY_SIZE(mnames));
+   TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);
+
+   /* Failure Test: valid array and higher count size than array size*/
+   err = rte_metrics_reg_names(&mnames[0], ARRAY_SIZE(mnames) + 2);
+   TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);


rte_metrics_reg_names() relies on cnt_names being truthful about the 
size of *names. If it is overstated there is nothing to prevent an array 
overrun and in most cases a core-dump.



+
+   /* Failure Test: valid array and count size lessthan 1*/
+   err = rte_metrics_reg_names(&mnames[0], 0);
+   TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
+
+   /* Failure Test: valid array and count exceeds max count  */
+   err = rte_metrics_reg_names(&mnames[0], INVALID_COUNT);
+   TEST_ASSERT(err == -ENOMEM, "%s, %d", __func__, __LINE__);


See previous comment.


+
+   /* Failure Test: Invalid array and valid count size */
+   err = rte_metrics_reg_names(NULL, ARRAY_SIZE(mnames) + 2);
+   TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
+
+   /* Failure Test: Valid array and Invalid count size */
+   err = rte_metrics_reg_names(&mnames[0], INVAL_METRIC_COUNT);
+   TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);


Ditto.


+
+   return TEST_SUCCESS;
+}
+


[..]


+/* Test case to validate update a list of  metrics  */
+static int
+test_metrics_update_values(void)
+{
+   int err = 0;
+   const uint64_t  value[REG_METRIC_COUNT] = {1, 2, 3, 4, 5, 6};
+
+   /* Success Test: valid data */
+   err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
+KEY, &value[0], ARRAY_SIZE(value));
+   TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);


Updates cannot overlap more than one metric set. A successful test would be:

/* Success Test: valid data */
err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 0,
&value[0], 1);
TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);
err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 1,
&value[1], 1);
TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);
err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 2,
&value[2], 4);
TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);

I am wondering whether this constraint should be removed.



+   /* Failed Test: Invalid count size */
+   err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
+KEY, &value[0], 0);
+   TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);


Good catch. Will update library code.



+
+   /* Success Test: valid data with lower count stats size */
+   err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
+KEY, &value[0], ARRAY_SIZE(value) - 3);
+   TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);


KEY should be 2, which is where the set of size four starts. The set 
starting at KEY (i.e 1) is of size one.




+
+   /* Failed Test: Invalid port_id(lower value) and valid data */
+   err = rte_metrics_update_values(-2, KEY, &value[0], ARRAY_SIZE(value));
+   TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
+
+   /* Failed Test: Invalid port_id(higher value) and valid data */
+   err = rte_metrics_update_values(39, 1, &value[0], ARRAY_SIZE(value));
+   TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);


Testcases Ok.


+
+   /* Failed Test: higher count size */
+   err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
+KEY, &value[0], ARRAY_SIZE(value) + 5);
+   TEST_ASSERT(err == -ERANGE, "%s, %d", __func__, __LINE__);


Array sizes should not be overstated.


+
+   /* Failed Test: Invalid array */
+   err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
+KEY, NULL, ARRAY_SIZE(value));
+   TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
+
+   return TEST_SUCCESS;
+}




+
+/* Test to validate get metric name-key lookup table */
+static int
+test_metrics_get_names(void)
+{

[..]


+
+   /* Failure Test: Invalid array list, Correct Count Stats same as
+* memzone stats
+*/
+   err = rte_metrics_get_names(metrics, REG_METRIC_COUNT);
+   TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);


Overstated array size.


+
+   /* Failure Test: Invalid array list, Increa

[dpdk-dev] [PATCH v1] lib/metrics: fix silent fail when uninitialised

[Remy Horton]

If rte_metrics_init() had not been called and hence the internal
metric storage is not allocated, rte_metrics_get_values() and
rte_metrics_get_name() would silently fail by returning zero
(i.e. no metrics registered). This patch changes the result of
this scenario to an explicit fail by returning -EIO.

Fixes: 349950ddb9c5 ("metrics: add information metrics library")

Signed-off-by: Remy Horton 
---
 lib/librte_metrics/rte_metrics.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index 258f058..fed0a33 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -200,9 +200,8 @@ rte_metrics_get_names(struct rte_metric_name *names,
int return_value;
 
memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
-   /* If not allocated, fail silently */
if (memzone == NULL)
-   return 0;
+   return -EIO;
 
stats = memzone->addr;
rte_spinlock_lock(&stats->lock);
@@ -238,9 +237,9 @@ rte_metrics_get_values(int port_id,
return -EINVAL;
 
memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
-   /* If not allocated, fail silently */
if (memzone == NULL)
-   return 0;
+   return -EIO;
+
stats = memzone->addr;
rte_spinlock_lock(&stats->lock);
 
-- 
2.9.5

[dpdk-dev] [PATCH v1] lib/metrics: disallow NULL as metric name

[Remy Horton]

This patch adds a sanity check so that names passed into
rte_metrics_reg_names() and the wrapper rte_metrics_reg_name()
cannot be NULL.

Fixes: 349950ddb9c5 ("metrics: add information metrics library")

Signed-off-by: Remy Horton 
---
 lib/librte_metrics/rte_metrics.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index 258f058..2275244 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -96,6 +96,9 @@ rte_metrics_reg_names(const char * const *names, uint16_t 
cnt_names)
/* Some sanity checks */
if (cnt_names < 1 || names == NULL)
return -EINVAL;
+   for (idx_name = 0; idx_name < cnt_names; idx_name++)
+   if (names[idx_name] == NULL)
+   return -EINVAL;
 
memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
if (memzone == NULL)
-- 
2.9.5

[dpdk-dev] [PATCH v1] lib/bitratestats: add NULL sanity check

[Remy Horton]

If rte_stats_bitrate_reg() is passed NULL, the result is a crash.
Fixed by adding a sanity check that makes sure the passed-in
pointer is not NULL.

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

Signed-off-by: Remy Horton 
---
 lib/librte_bitratestats/rte_bitrate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_bitratestats/rte_bitrate.c 
b/lib/librte_bitratestats/rte_bitrate.c
index 964e3c3..20e1755 100644
--- a/lib/librte_bitratestats/rte_bitrate.c
+++ b/lib/librte_bitratestats/rte_bitrate.c
@@ -47,6 +47,9 @@ rte_stats_bitrate_reg(struct rte_stats_bitrates *bitrate_data)
};
int return_value;
 
+   if (bitrate_data == NULL)
+   return -EINVAL;
+
return_value = rte_metrics_reg_names(&names[0], ARRAY_SIZE(names));
if (return_value >= 0)
bitrate_data->id_stats_set = return_value;
-- 
2.9.5

Re: [dpdk-dev] [PATCH v4 02/24] bus/vdev: enable one device scan

[Remy Horton]

On 26/06/2018 08:08, Qi Zhang wrote:

The patch implemented the ops scan_one for vdev bus, it gives two benifits
1. Improve scan efficiency when a device is attached as hotplug, since no
need to pupulate a new device by iterating all devargs in devargs_list.
2. It also avoid sync IPC invoke (which happens in vdev->scan on secondary
process). The benifit is this removes the potential deadlock in the case
when secondary process receive a request from primary process to attach a
new device, since vdev->scan will be invoked on mp thread itself in that
case.


Slight rewording and spelling corrections within description:

The patch implements the ops scan_one for the vdev bus, which gives two 
benefits:
1. Improves scan efficiency when a device is attached as hotplug, since 
there is no need to populate a new device by iterating all devargs in 
devargs_list
2. It also avoids sync IPC invoke (which happens in vdev->scan on 
secondary process). The benefit is this removes the potential deadlock 
in the case when a secondary process receives a request from primary 
process to attach a new device, since vdev->scan will be invoked on mp 
thread itself in that case.




Signed-off-by: Qi Zhang 
---

 drivers/bus/vdev/vdev.c | 30 ++
 1 file changed, 30 insertions(+)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v4 01/24] eal: introduce one device scan

[Remy Horton]

On 26/06/2018 08:08, Qi Zhang wrote:
[..]

Signed-off-by: Qi Zhang 
---

 lib/librte_eal/common/eal_common_dev.c  | 17 +
 lib/librte_eal/common/include/rte_bus.h | 16 


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v4 03/24] ethdev: add function to release port in local process

[Remy Horton]

On 26/06/2018 08:08, Qi Zhang wrote:
[..]

Signed-off-by: Qi Zhang 
---

 lib/librte_ethdev/rte_ethdev.c| 24 +---
 lib/librte_ethdev/rte_ethdev_driver.h | 13 +
 2 files changed, 34 insertions(+), 3 deletions(-)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v4 09/24] net/i40e: enable port detach on secondary process

[Remy Horton]

On 26/06/2018 08:08, Qi Zhang wrote:
[..]

 static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 {
+   struct rte_eth_dev *ethdev;
+   ethdev = rte_eth_dev_allocated(pci_dev->device.name);
+
+   if (!ethdev)
+   return -ENODEV;
+
+   if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+   return rte_eth_dev_release_port_private(ethdev);
+
return rte_eth_dev_pci_generic_remove(pci_dev, i40evf_dev_uninit);
 }


This identical code appears in multiple drivers. Is there anything 
stopping it being folded into rte_eth_dev_pci_generic_remove()?

Re: [dpdk-dev] [PATCH v4 10/24] net/ixgbe: enable port detach on secondary process

[Remy Horton]

On 26/06/2018 08:08, Qi Zhang wrote:
[..]

 static int eth_ixgbevf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
struct rte_pci_device *pci_dev)
 {
+   struct rte_eth_dev *ethdev;
+
+   ethdev = rte_eth_dev_allocated(pci_dev->device.name);
+   if (!ethdev)
+   return -ENODEV;
+
+   if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+   return rte_eth_dev_release_port_private(ethdev);
+
return rte_eth_dev_pci_generic_probe(pci_dev,
sizeof(struct ixgbe_adapter), eth_ixgbevf_dev_init);
 }



Is calling of rte_eth_dev_release_port_private() from the probe function 
intentional? To me it looks like the code has been pasted into the wrong 
place..

Re: [dpdk-dev] [DPDK] examples/ipsec-secgw: fix use of unsupported RSS offloads

[Remy Horton]

On 26/06/2018 10:03, Ferruh Yigit wrote:
[..]

Hi Remy,

Is following covering this patch:
https://patches.dpdk.org/patch/41313/


Patch was sent out with wrong subject, so consider it Nack'd.

Re: [dpdk-dev] [PATCH v1] examples/ipsec-secgw: fix use of unsupported RSS offloads

[Remy Horton]

On 22/06/2018 14:36, Remy Horton wrote:
[..]

Signed-off-by: Remy Horton 
---
 examples/ipsec-secgw/ipsec-secgw.c | 5 +
 1 file changed, 5 insertions(+)


Self-Nack. Another pending patch already fixes this.

Re: [dpdk-dev] [DPDK] examples/ipsec-secgw: fix use of unsupported RSS offloads

[Remy Horton]

On 22/06/2018 16:46, Ferruh Yigit wrote:
[..]

Is following covering this patch:
https://patches.dpdk.org/patch/41313/


Looks like it. Didn't spot it earlier..

..Remy

[dpdk-dev] [PATCH v1] examples/ipsec-secgw: fix use of unsupported RSS offloads

[Remy Horton]

Since commit aa1a6d87f15d ("ethdev: force RSS offload rules again")
a check that requested RSS offloads are supported by a PMD is
enforced, whereas in the past asking for unsupported offloads would
not result in an error. This patch changes the IPSec gateway sample
so that it only requests modes that are supported rather than
failing to start up.

Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")

Signed-off-by: Remy Horton 
---
 examples/ipsec-secgw/ipsec-secgw.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/examples/ipsec-secgw/ipsec-secgw.c 
b/examples/ipsec-secgw/ipsec-secgw.c
index a5da8b2..d247d5f 100644
--- a/examples/ipsec-secgw/ipsec-secgw.c
+++ b/examples/ipsec-secgw/ipsec-secgw.c
@@ -1566,6 +1566,11 @@ port_init(uint16_t portid)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_MBUF_FAST_FREE)
local_port_conf.txmode.offloads |=
DEV_TX_OFFLOAD_MBUF_FAST_FREE;
+
+   /* Only request RSS offloads the NIC supports. */
+   local_port_conf.rx_adv_conf.rss_conf.rss_hf &=
+   dev_info.flow_type_rss_offloads;
+
ret = rte_eth_dev_configure(portid, nb_rx_queue, nb_tx_queue,
&local_port_conf);
if (ret < 0)
-- 
2.9.5

[dpdk-dev] [DPDK] examples/ipsec-secgw: fix use of unsupported RSS offloads

[Remy Horton]

Since commit aa1a6d87f15d ("ethdev: force RSS offload rules again")
a check that requested RSS offloads are supported by a PMD is
enforced, whereas in the past asking for unsupported offloads would
not result in an error. This patch changes the IPSec gateway sample
so that it only requests modes that are supported rather than
failing to start up.

Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")

Signed-off-by: Remy Horton 
---
 examples/ipsec-secgw/ipsec-secgw.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/examples/ipsec-secgw/ipsec-secgw.c 
b/examples/ipsec-secgw/ipsec-secgw.c
index a5da8b2..d247d5f 100644
--- a/examples/ipsec-secgw/ipsec-secgw.c
+++ b/examples/ipsec-secgw/ipsec-secgw.c
@@ -1566,6 +1566,11 @@ port_init(uint16_t portid)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_MBUF_FAST_FREE)
local_port_conf.txmode.offloads |=
DEV_TX_OFFLOAD_MBUF_FAST_FREE;
+
+   /* Only request RSS offloads the NIC supports. */
+   local_port_conf.rx_adv_conf.rss_conf.rss_hf &=
+   dev_info.flow_type_rss_offloads;
+
ret = rte_eth_dev_configure(portid, nb_rx_queue, nb_tx_queue,
&local_port_conf);
if (ret < 0)
-- 
2.9.5

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Remy Horton]

On 14/06/2018 11:55, Ferruh Yigit wrote:

On 6/14/2018 7:39 AM, Remy Horton wrote:


On 13/06/2018 16:39, Ferruh Yigit wrote:

On 6/7/2018 9:15 AM, David Marchand wrote:

Testpmd should not expect the xstats names and values arrays to be
aligned: neither the arrays sizes, nor the order in which the values are.

[..]


APIs exist for getting subset of values (.._by_id) but they both assume
requested ids are array index.
As you said this works fine because of xstats[idx].id==idx


Changing that coincidence into an assumption looks like a bug to me.



struct rte_eth_xstat_name { char name[]; }
struct rte_eth_xstat { uint64_t id; uint64_t value; }

These two structs are for basic key-value match.
But one has the "id" field, but other doesn't. If we use "id" as match, this
will be the index of xstat_name[]. This is extra complexity, and xstats is
already unnecessarily complex.

I am for documenting that "xstat_name" and "xstat" are aligned, both in size and
order, and array indexes are ids, clearly in API doc and continue with existing
implementation. What do you think?


As long as none of the PMD vendors intend to take advantage of 
xstats[idx].id!=idx (e.g. to allow omitted values) then I'm OK with it.



..Remy

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Remy Horton]

On 13/06/2018 16:39, Ferruh Yigit wrote:

On 6/7/2018 9:15 AM, David Marchand wrote:

Testpmd should not expect the xstats names and values arrays to be
aligned: neither the arrays sizes, nor the order in which the values are.


As far as I can see this assumption is everywhere in API implementation:
xstats names and values are aligned with same order.
The basic stat part of the xstats, implemented in ethdev layer, seems relying on
same assumption. Also looks like "xstat size" and "xstat_names size" used
interchangeably.

And I don't see any case that mentions xstats.id is xstats_name index.
cc'ed Harry, to get more information about initial intention.

the id value in xstats struct looks like duplication, but other than that, is
there any downside of using array index to mach name, value pair?
And do we really need another layer of indirection (and complexity) to mach
simple name,value key pair in xstats?


When I was working on xstats one of my intentions was to allow PMDs to 
only return a subset of values for all the keys they declare, with 
xstats[idx].id==idx just being a coincidence that was not to be relied 
on. Since then there appears to have been several instances of rework, 
so no idea if this coincidence becoming an assumption was intentional or 
an oversight.


..Remy

Re: [dpdk-dev] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Remy Horton]

Was out of office, so only saw the patchset this morning.

Missing fixlines (watch out for subject line wrap):

Fixes: e2aae1c1ced9 ("ethdev: remove name from extended statistic fetch")
Fixes: 0a5beecf466a ("ethdev: revert xstats by ID")

Otherwise looks good to me. A case of implementation simplification 
turning into API assumption :(


..Remy


On 07/06/2018 09:15, David Marchand wrote:
[..]

Signed-off-by: David Marchand 


Acked-by: Remy Horton 

[dpdk-dev] [PATCH v1] net/ixgbe: add tuned RxTx parmeters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch adds the values for the ixgbe PMD.

Signed-off-by: Remy Horton 
---
 doc/guides/rel_notes/release_18_08.rst | 8 
 drivers/net/ixgbe/ixgbe_ethdev.c   | 8 
 2 files changed, 16 insertions(+)

diff --git a/doc/guides/rel_notes/release_18_08.rst 
b/doc/guides/rel_notes/release_18_08.rst
index 5bc23c5..17e6ff9 100644
--- a/doc/guides/rel_notes/release_18_08.rst
+++ b/doc/guides/rel_notes/release_18_08.rst
@@ -41,6 +41,14 @@ New Features
  Also, make sure to start the actual text at the margin.
  =
 
+* **Added ixgbe preferred Rx/Tx parameters.**
+
+  Rather than applications providing explicit Rx and Tx parameters such as
+  queue and burst sizes, they can request that the EAL instead uses preferred
+  values provided by the PMD, falling back to defaults within the EAL if the
+  PMD does not provide any. The provision of such tuned values now includes
+  the ixgbe PMD.
+
 
 API Changes
 ---
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 87d2ad0..95fd080 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -3755,6 +3755,14 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->speed_capa |= ETH_LINK_SPEED_2_5G;
dev_info->speed_capa |= ETH_LINK_SPEED_5G;
}
+
+   /* Driver-preferred Rx/Tx parameters */
+   dev_info->default_rxportconf.burst_size = 32;
+   dev_info->default_txportconf.burst_size = 32;
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
 }
 
 static const uint32_t *
-- 
2.9.5

Re: [dpdk-dev] [PATCH] net/ixgbe: fix crash on detach

[Remy Horton]

On 30/05/2018 07:31, Pablo de Lara wrote:
[..]

Reported-by: Anatoly Burakov 
Signed-off-by: Pablo de Lara 


Acked-by: Remy Horton 

[dpdk-dev] [PATCH v2] i40e: fix corruption of VF stats via port representor

[Remy Horton]

The resetting of stats on a VF involves the setting of an offset
that is subtracted from future calls, rather zeroing of counters.
However doing a stats reset on the port representor was also
adjusting the values forwarded to the VF, which had the effect of
corrupting the VF's counters. The fix is for the port representor
to maintain its own stats offset, so the port representor and VF
maintain independently-resettable counters.

Fixes: e0cb96204b71 ("net/i40e: add support for representor ports")

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_ethdev.h |   2 +
 drivers/net/i40e/i40e_vf_representor.c | 119 +++--
 2 files changed, 117 insertions(+), 4 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 55c8875..11c4c76 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -1083,6 +1083,8 @@ struct i40e_vf_representor {
/**< Virtual Function ID */
struct i40e_adapter *adapter;
/**< Private data store of assocaiated physical function */
+   struct i40e_eth_stats stats_offset;
+   /**< Zero-point of VF statistics*/
 };
 
 extern const struct rte_flow_ops i40e_flow_ops;
diff --git a/drivers/net/i40e/i40e_vf_representor.c 
b/drivers/net/i40e/i40e_vf_representor.c
index 6026ec9..8dc94b4 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -139,15 +139,126 @@ i40e_vf_representor_tx_queue_setup(__rte_unused struct 
rte_eth_dev *dev,
return 0;
 }
 
+static void
+i40evf_stat_update_48(uint64_t *offset,
+  uint64_t *stat)
+{
+   if (*stat >= *offset)
+   *stat = *stat - *offset;
+   else
+   *stat = (uint64_t)((*stat +
+   ((uint64_t)1 << I40E_48_BIT_WIDTH)) - *offset);
+
+   *stat &= I40E_48_BIT_MASK;
+}
+
+static void
+i40evf_stat_update_32(uint64_t *offset,
+  uint64_t *stat)
+{
+   if (*stat >= *offset)
+   *stat = (uint64_t)(*stat - *offset);
+   else
+   *stat = (uint64_t)((*stat +
+   ((uint64_t)1 << I40E_32_BIT_WIDTH)) - *offset);
+}
+
+static int
+rte_pmd_i40e_get_vf_native_stats(uint16_t port,
+ uint16_t vf_id,
+ struct i40e_eth_stats *stats)
+{
+   struct rte_eth_dev *dev;
+   struct i40e_pf *pf;
+   struct i40e_vsi *vsi;
+
+   RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
+
+   dev = &rte_eth_devices[port];
+
+   if (!is_i40e_supported(dev))
+   return -ENOTSUP;
+
+   pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+
+   if (vf_id >= pf->vf_num || !pf->vfs) {
+   PMD_DRV_LOG(ERR, "Invalid VF ID.");
+   return -EINVAL;
+   }
+
+   vsi = pf->vfs[vf_id].vsi;
+   if (!vsi) {
+   PMD_DRV_LOG(ERR, "Invalid VSI.");
+   return -EINVAL;
+   }
+
+   i40e_update_vsi_stats(vsi);
+   memcpy(stats, &vsi->eth_stats, sizeof(vsi->eth_stats));
+
+   return 0;
+}
+
 static int
 i40e_vf_representor_stats_get(struct rte_eth_dev *ethdev,
struct rte_eth_stats *stats)
 {
struct i40e_vf_representor *representor = ethdev->data->dev_private;
+   struct i40e_eth_stats native_stats;
+   int ret;
 
-   return rte_pmd_i40e_get_vf_stats(
+   ret = rte_pmd_i40e_get_vf_native_stats(
representor->adapter->eth_dev->data->port_id,
-   representor->vf_id, stats);
+   representor->vf_id, &native_stats);
+   if (ret == 0) {
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_bytes,
+   &native_stats.rx_bytes);
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_unicast,
+   &native_stats.rx_unicast);
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_multicast,
+   &native_stats.rx_multicast);
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_broadcast,
+   &native_stats.rx_broadcast);
+   i40evf_stat_update_32(
+   &representor->stats_offset.rx_discards,
+   &native_stats.rx_discards);
+   i40evf_stat_update_32(
+   &representor->stats_offset.rx_unknown_protocol,
+   &native_stats.rx_unknown_protocol);
+   i40evf_stat_update_48(
+   &representor->stats_offset.tx_bytes,
+   &native_stats.tx_bytes);
+   i40evf_stat_update_48(
+

Re: [dpdk-dev] [PATCH] net/i40e: fix corruption of VF stats via port representor

[Remy Horton]

Missing fixline, so v2 coming..

On 18/05/2018 08:59, Remy Horton wrote:

The resetting of stats on a VF involves the setting of an offset
that is subtracted from future calls, rather zeroing of counters.
However doing a stats reset on the port representor was also
adjusting the values forwarded to the VF, which had the effect of
corrupting the VF's counters. The fix is for the port representor
to maintain its own stats offset, so the port representor and VF
maintain independently-resettable counters

Signed-off-by: Remy Horton 

[dpdk-dev] [PATCH] net/i40e: fix corruption of VF stats via port representor

[Remy Horton]

The resetting of stats on a VF involves the setting of an offset
that is subtracted from future calls, rather zeroing of counters.
However doing a stats reset on the port representor was also
adjusting the values forwarded to the VF, which had the effect of
corrupting the VF's counters. The fix is for the port representor
to maintain its own stats offset, so the port representor and VF
maintain independently-resettable counters

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_ethdev.h |   2 +
 drivers/net/i40e/i40e_vf_representor.c | 119 +++--
 2 files changed, 117 insertions(+), 4 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 55c8875..11c4c76 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -1083,6 +1083,8 @@ struct i40e_vf_representor {
/**< Virtual Function ID */
struct i40e_adapter *adapter;
/**< Private data store of assocaiated physical function */
+   struct i40e_eth_stats stats_offset;
+   /**< Zero-point of VF statistics*/
 };
 
 extern const struct rte_flow_ops i40e_flow_ops;
diff --git a/drivers/net/i40e/i40e_vf_representor.c 
b/drivers/net/i40e/i40e_vf_representor.c
index 6026ec9..8dc94b4 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -139,15 +139,126 @@ i40e_vf_representor_tx_queue_setup(__rte_unused struct 
rte_eth_dev *dev,
return 0;
 }
 
+static void
+i40evf_stat_update_48(uint64_t *offset,
+  uint64_t *stat)
+{
+   if (*stat >= *offset)
+   *stat = *stat - *offset;
+   else
+   *stat = (uint64_t)((*stat +
+   ((uint64_t)1 << I40E_48_BIT_WIDTH)) - *offset);
+
+   *stat &= I40E_48_BIT_MASK;
+}
+
+static void
+i40evf_stat_update_32(uint64_t *offset,
+  uint64_t *stat)
+{
+   if (*stat >= *offset)
+   *stat = (uint64_t)(*stat - *offset);
+   else
+   *stat = (uint64_t)((*stat +
+   ((uint64_t)1 << I40E_32_BIT_WIDTH)) - *offset);
+}
+
+static int
+rte_pmd_i40e_get_vf_native_stats(uint16_t port,
+ uint16_t vf_id,
+ struct i40e_eth_stats *stats)
+{
+   struct rte_eth_dev *dev;
+   struct i40e_pf *pf;
+   struct i40e_vsi *vsi;
+
+   RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
+
+   dev = &rte_eth_devices[port];
+
+   if (!is_i40e_supported(dev))
+   return -ENOTSUP;
+
+   pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+
+   if (vf_id >= pf->vf_num || !pf->vfs) {
+   PMD_DRV_LOG(ERR, "Invalid VF ID.");
+   return -EINVAL;
+   }
+
+   vsi = pf->vfs[vf_id].vsi;
+   if (!vsi) {
+   PMD_DRV_LOG(ERR, "Invalid VSI.");
+   return -EINVAL;
+   }
+
+   i40e_update_vsi_stats(vsi);
+   memcpy(stats, &vsi->eth_stats, sizeof(vsi->eth_stats));
+
+   return 0;
+}
+
 static int
 i40e_vf_representor_stats_get(struct rte_eth_dev *ethdev,
struct rte_eth_stats *stats)
 {
struct i40e_vf_representor *representor = ethdev->data->dev_private;
+   struct i40e_eth_stats native_stats;
+   int ret;
 
-   return rte_pmd_i40e_get_vf_stats(
+   ret = rte_pmd_i40e_get_vf_native_stats(
representor->adapter->eth_dev->data->port_id,
-   representor->vf_id, stats);
+   representor->vf_id, &native_stats);
+   if (ret == 0) {
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_bytes,
+   &native_stats.rx_bytes);
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_unicast,
+   &native_stats.rx_unicast);
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_multicast,
+   &native_stats.rx_multicast);
+   i40evf_stat_update_48(
+   &representor->stats_offset.rx_broadcast,
+   &native_stats.rx_broadcast);
+   i40evf_stat_update_32(
+   &representor->stats_offset.rx_discards,
+   &native_stats.rx_discards);
+   i40evf_stat_update_32(
+   &representor->stats_offset.rx_unknown_protocol,
+   &native_stats.rx_unknown_protocol);
+   i40evf_stat_update_48(
+   &representor->stats_offset.tx_bytes,
+   &native_stats.tx_bytes);
+   i40evf_stat_update_48(
+   &representor->stats_offset.tx_unicast,
+

Re: [dpdk-dev] [PATCH v3 3/3] memzone: improve zero-length memzone reserve

[Remy Horton]

On 14/05/2018 12:29, Burakov, Anatoly wrote:
[..]

This failure is not caused by this patchset, and you should get similar
failures on master if you get these while testing my patchset. I am not
able to reproduce this issue, but i'll double-check the bounded reserve
code with a fine-toothed comb anyway.


I reliably get the failure with V3 applied to RC2, but so far haven't 
been able to replicate with any other combination (clean RC2, RC2+V5, 
master+V4, etc). Odd..

Re: [dpdk-dev] [PATCH] net/i40e: fix missing Port Representor data-path callbacks

[Remy Horton]

On 11/05/2018 11:48, Mohammad Abdul Awal wrote:
[..]

+static uint16_t
+i40e_vf_representor_tx_burst(__rte_unused void *tx_queue,
+struct rte_mbuf **tx_pkts,
+uint16_t nb_pkts)
+{
+uint16_t idx_pkt;
+
+for (idx_pkt = 0; idx_pkt < nb_pkts; idx_pkt++)
+rte_pktmbuf_free(tx_pkts[idx_pkt]);

We should not free them in the driver silently whereas the application
will think that it has been sent successfully.
Please use the same rule for rx_burst, and return 0.


Ok, v2 coming.

I'll also combine this and the ixgbe patch into the same patchset.

[dpdk-dev] [PATCH v2 2/2] net/ixgbe: fix missing Port Representor data-path

[Remy Horton]

This patch adds Rx and Tx burst functions to the ixgbe
Port Representors, so that the implementation within
ixgbe PMD can be tested using applications such as
testpmd which require data-path functionality.

Fixes: cf80ba6e2038 ("net/ixgbe: add support for representor ports")

Signed-off-by: Remy Horton 
---
 drivers/net/ixgbe/ixgbe_vf_representor.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c 
b/drivers/net/ixgbe/ixgbe_vf_representor.c
index 8f8af49..db516d9 100644
--- a/drivers/net/ixgbe/ixgbe_vf_representor.c
+++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
@@ -153,6 +153,20 @@ struct eth_dev_ops ixgbe_vf_representor_dev_ops = {
.mac_addr_set   = ixgbe_vf_representor_mac_addr_set,
 };
 
+static uint16_t
+ixgbe_vf_representor_rx_burst(__rte_unused void *rx_queue,
+   __rte_unused struct rte_mbuf **rx_pkts, __rte_unused uint16_t nb_pkts)
+{
+   return 0;
+}
+
+static uint16_t
+ixgbe_vf_representor_tx_burst(__rte_unused void *tx_queue,
+   __rte_unused struct rte_mbuf **tx_pkts, __rte_unused uint16_t nb_pkts)
+{
+   return 0;
+}
+
 int
 ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 {
@@ -182,9 +196,11 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void 
*init_params)
/* Set representor device ops */
ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
 
-   /* No data-path so no RX/TX functions */
-   ethdev->rx_pkt_burst = NULL;
-   ethdev->tx_pkt_burst = NULL;
+   /* No data-path, but need stub Rx/Tx functions to avoid crash
+* when testing with the likes of testpmd.
+*/
+   ethdev->rx_pkt_burst = ixgbe_vf_representor_rx_burst;
+   ethdev->tx_pkt_burst = ixgbe_vf_representor_tx_burst;
 
/* Setting the number queues allocated to the VF */
ethdev->data->nb_rx_queues = IXGBE_VF_MAX_RX_QUEUES;
-- 
2.9.5

[dpdk-dev] [PATCH v2 1/2] net/i40e: fix missing Port Representor data-path

[Remy Horton]

This patch adds Rx and Tx burst functions to the i40e Port
Representors, so that the implementation within this PMD
can be tested using applications such as testpmd which
require data-path functionality.

Fixes: e0cb96204b71 ("net/i40e: add support for representor ports")

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_vf_representor.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c 
b/drivers/net/i40e/i40e_vf_representor.c
index 96b3787..6026ec9 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -337,6 +337,20 @@ struct eth_dev_ops i40e_representor_dev_ops = {
 
 };
 
+static uint16_t
+i40e_vf_representor_rx_burst(__rte_unused void *rx_queue,
+   __rte_unused struct rte_mbuf **rx_pkts, __rte_unused uint16_t nb_pkts)
+{
+   return 0;
+}
+
+static uint16_t
+i40e_vf_representor_tx_burst(__rte_unused void *tx_queue,
+   __rte_unused struct rte_mbuf **tx_pkts, __rte_unused uint16_t nb_pkts)
+{
+   return 0;
+}
+
 int
 i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 {
@@ -365,9 +379,11 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void 
*init_params)
/* Set representor device ops */
ethdev->dev_ops = &i40e_representor_dev_ops;
 
-   /* No data-path so no RX/TX functions */
-   ethdev->rx_pkt_burst = NULL;
-   ethdev->tx_pkt_burst = NULL;
+   /* No data-path, but need stub Rx/Tx functions to avoid crash
+* when testing with the likes of testpmd.
+*/
+   ethdev->rx_pkt_burst = i40e_vf_representor_rx_burst;
+   ethdev->tx_pkt_burst = i40e_vf_representor_tx_burst;
 
vf = &pf->vfs[representor->vf_id];
 
-- 
2.9.5

[dpdk-dev] [PATCH v2 0/2] fix missing Port Representor data-path callbacks

[Remy Horton]

This patchset adds Rx and Tx burst functions to the ixgbe and i40e Port
Representors, so that the implementation within these PMDd can be tested
using applications such as testpmd which require data-path functionality.

Changes in v2:
* Combine patches into one patchset
* Tx burst changed from silently consuming packets to No-Op

Remy Horton (2):
  net/i40e: fix missing Port Representor data-path
  net/ixgbe: fix missing Port Representor data-path

 drivers/net/i40e/i40e_vf_representor.c   | 22 +++---
 drivers/net/ixgbe/ixgbe_vf_representor.c | 22 +++---
 2 files changed, 38 insertions(+), 6 deletions(-)

-- 
2.9.5

Re: [dpdk-dev] [PATCH v3 3/3] memzone: improve zero-length memzone reserve

[Remy Horton]

On 03/05/2018 18:18, Anatoly Burakov wrote:
[..]

Also, fixup unit tests to account for the new behavior.


Tried running the tests but it fails on a boundary check:

RTE>>memzone_autotest
test basic memzone API
[...]
1GB Huge pages available
test alignment for memzone_reserve
check alignments and lengths
check overlapping
test boundary alignment for memzone_reserve
check_memzone_bounded(MZ_TEST_bounded_128): invalid memzone boundary 128 
crossed

Test Failed
RTE>>

Re: [dpdk-dev] [PATCH v3 2/3] malloc: allow reserving biggest element

[Remy Horton]

On 03/05/2018 18:18, Anatoly Burakov wrote:
[..]

Signed-off-by: Anatoly Burakov 


[..]

+   for (idx = 0; idx < RTE_HEAP_NUM_FREELISTS; idx++) {
+   for (elem = LIST_FIRST(&heap->free_head[idx]);
+   !!elem; elem = LIST_NEXT(elem, free_list)) {


Why the double-negation?

Otherwise, see no issues.

Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v3 1/3] malloc: add biggest free IOVA-contiguous element to stats

[Remy Horton]

Logic looks ok to me.

On 03/05/2018 18:17, Anatoly Burakov wrote:

User might be interested to find out what is the biggest chunk of
IOVA-contiguous free space that can be allocated from malloc. Add
relevant malloc-internal functions and expose this through malloc
stats calculation call.

Signed-off-by: Anatoly Burakov 


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH] eventdev: change port_id to uint16_t

[Remy Horton]

Would have preferred it if the patch also changed the variable name to 
something a bit more descriptive (e.g. port_id, idx_port, ..).


On 08/05/2018 15:27, Lei wrote:

From: yao 

From 17.11, port_id is changed from uint8_t to uint16_t.But in eventdev,
it still use the old fashion. This patch fix this issue.


Reworded using proper tense:

In 17.11 port_id was changed from uint8_t to uint16_t, but eventdev
still uses the old fashion. This patch fixes this issue.



Signed-off-by: Lei Yao 


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: make locking memory configurable

[Remy Horton]

Looks good to me, and passed quick smoke test.

On 03/05/2018 13:38, Anatoly Burakov wrote:
[..]

Signed-off-by: Jianfeng Tan 
Signed-off-by: Anatoly Burakov 


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix device configure with zero queue

[Remy Horton]

On 10/05/2018 03:22, Qi Zhang wrote:
[..]

Fixes: 3be82f5cc5e ("ethdev: support PMD-tuned Tx/Rx parameters")
Signed-off-by: Qi Zhang 


Acked-by: Remy Horton 

[dpdk-dev] [DPDK] net/ixgbe: fix missing Port Representor data-path callbacks

[Remy Horton]

This patch adds Rx and Tx burst functions to the ixgbe
Port Representors, so that the implementation within
ixgbe PMD can be tested using applications such as
testpmd which require data-path functionality.

Fixes: cf80ba6e2038 ("net/ixgbe: add support for representor ports")

Signed-off-by: Remy Horton 
---
 drivers/net/ixgbe/ixgbe_vf_representor.c | 27 ---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c 
b/drivers/net/ixgbe/ixgbe_vf_representor.c
index 8f8af49..21c44d1 100644
--- a/drivers/net/ixgbe/ixgbe_vf_representor.c
+++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
@@ -153,6 +153,25 @@ struct eth_dev_ops ixgbe_vf_representor_dev_ops = {
.mac_addr_set   = ixgbe_vf_representor_mac_addr_set,
 };
 
+static uint16_t
+ixgbe_vf_representor_rx_burst(__rte_unused void *rx_queue,
+   __rte_unused struct rte_mbuf **rx_pkts, __rte_unused uint16_t nb_pkts)
+{
+   return 0;
+}
+
+static uint16_t
+ixgbe_vf_representor_tx_burst(__rte_unused void *tx_queue,
+   struct rte_mbuf **tx_pkts,
+   uint16_t nb_pkts)
+{
+   uint16_t idx_pkt;
+
+   for (idx_pkt = 0; idx_pkt < nb_pkts; idx_pkt++)
+   rte_pktmbuf_free(tx_pkts[idx_pkt]);
+   return nb_pkts;
+}
+
 int
 ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 {
@@ -182,9 +201,11 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void 
*init_params)
/* Set representor device ops */
ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
 
-   /* No data-path so no RX/TX functions */
-   ethdev->rx_pkt_burst = NULL;
-   ethdev->tx_pkt_burst = NULL;
+   /* No data-path, but need stub Rx/Tx functions to avoid crash
+* when testing with the likes of testpmd.
+*/
+   ethdev->rx_pkt_burst = ixgbe_vf_representor_rx_burst;
+   ethdev->tx_pkt_burst = ixgbe_vf_representor_tx_burst;
 
/* Setting the number queues allocated to the VF */
ethdev->data->nb_rx_queues = IXGBE_VF_MAX_RX_QUEUES;
-- 
2.9.5

[dpdk-dev] [PATCH] net/i40e: fix missing Port Representor data-path callbacks

[Remy Horton]

This patch adds Rx and Tx burst functions to the i40e Port
Representors, so that the implementation within this PMD
can be tested using applications such as testpmd which
require data-path functionality.

Fixes: e0cb96204b71 ("net/i40e: add support for representor ports")

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_vf_representor.c | 27 ---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c 
b/drivers/net/i40e/i40e_vf_representor.c
index 96b3787..e205019 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -337,6 +337,25 @@ struct eth_dev_ops i40e_representor_dev_ops = {
 
 };
 
+static uint16_t
+i40e_vf_representor_rx_burst(__rte_unused void *rx_queue,
+   __rte_unused struct rte_mbuf **rx_pkts, __rte_unused uint16_t nb_pkts)
+{
+   return 0;
+}
+
+static uint16_t
+i40e_vf_representor_tx_burst(__rte_unused void *tx_queue,
+   struct rte_mbuf **tx_pkts,
+   uint16_t nb_pkts)
+{
+   uint16_t idx_pkt;
+
+   for (idx_pkt = 0; idx_pkt < nb_pkts; idx_pkt++)
+   rte_pktmbuf_free(tx_pkts[idx_pkt]);
+   return nb_pkts;
+}
+
 int
 i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 {
@@ -365,9 +384,11 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void 
*init_params)
/* Set representor device ops */
ethdev->dev_ops = &i40e_representor_dev_ops;
 
-   /* No data-path so no RX/TX functions */
-   ethdev->rx_pkt_burst = NULL;
-   ethdev->tx_pkt_burst = NULL;
+   /* No data-path, but need stub Rx/Tx functions to avoid crash
+* when testing with the likes of testpmd.
+*/
+   ethdev->rx_pkt_burst = i40e_vf_representor_rx_burst;
+   ethdev->tx_pkt_burst = i40e_vf_representor_tx_burst;
 
vf = &pf->vfs[representor->vf_id];
 
-- 
2.9.5

Re: [dpdk-dev] [PATCH v8 6/9] ethdev: add common devargs parser

[Remy Horton]

On 26/04/2018 13:03, Ananyev, Konstantin wrote:
[..]

I still think that if you'd like to extend rte_kvarrgs to be able to parse something 
like: "key=[val1,val2,...,valn]",
you have to make it generic kvargs ability and put it into librte_kvargs, not 
try to introduce your own new parser here.
Imagine that in addition to your 'port=[val1,val2, ..valn]' devargs string 
would contain some extra (let say device specific)
parameters.
What would happen, when PMD will try to use rte_kvargs_parse() on such string?
My understanding - it would fail, correct?


This is partly dependent on what will (and won't) devargs provide when 
it is finalised. It was insourced in order to unblock the rest of the 
patchset in the meantime.

Re: [dpdk-dev] [PATCH v7 6/9] ethdev: add common devargs parser

[Remy Horton]

On 25/04/2018 11:06, Thomas Monjalon wrote:

25/04/2018 11:40, Remy Horton:


On 24/04/2018 20:53, Thomas Monjalon wrote:

[..]

OK
Please can you add it as a comment in the parsing code?

We will need one or two examples in the commit message too.


Docs are being updated, so it should be in the v8 patchset.

Re: [dpdk-dev] [PATCH v7 6/9] ethdev: add common devargs parser

[Remy Horton]

On 24/04/2018 20:53, Thomas Monjalon wrote:
[..]

But I would like to review the devargs you are standardizing.
Unfortunately, I cannot find a documentation about it.
How users are supposed to use it?
Can you, at least, describe the syntax in the commit log, please?


The patch follows this pseudo-BNF:

cfg   := pair (',' pair)*
pair  := (key '=' value)
key   := 'port' | 'representor'
value := range | list
range := int ('-' int)?
int   := [0-9]+
list  := '[' range (',' range)* ']'

Re: [dpdk-dev] [PATCH v3 0/5] get the information and data of EEPROM

[Remy Horton]

On 19/04/2018 14:36, Ferruh Yigit wrote:
[..]

Zijie Pan (5):
  ethdev: add access to eeprom
  examples/ethtool: add a new command module-eeprom
  net/ixgbe: add module EEPROM callbacks for ixgbe
  net/e1000: add module EEPROM callbacks for e1000
  net/i40e: add module EEPROM callbacks for i40e


This patch is waiting for a while without comment, please share if there is any
concern about patch.


Wasn't able to test the e1000/i40e callbacks but that aside the patchset 
looks good to me. Extend my previous ack to the whole series.

[dpdk-dev] [PATCH v6 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
Reviewed-by: Ferruh Yigit 
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0bf4e3..acf1f99 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3291,15 +3291,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->max_tx_queues += dev_info->vmdq_queue_num;
}
 
-   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
/* For XL710 */
dev_info->speed_capa = ETH_LINK_SPEED_40G;
-   else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+   dev_info->default_rxportconf.nb_queues = 2;
+   dev_info->default_txportconf.nb_queues = 2;
+   if (dev->data->nb_rx_queues == 1)
+   dev_info->default_rxportconf.ring_size = 2048;
+   else
+   dev_info->default_rxportconf.ring_size = 1024;
+   if (dev->data->nb_tx_queues == 1)
+   dev_info->default_txportconf.ring_size = 1024;
+   else
+   dev_info->default_txportconf.ring_size = 512;
+
+   } else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
/* For XXV710 */
dev_info->speed_capa = ETH_LINK_SPEED_25G;
-   else
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
/* For X710 */
dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+   dev_info->default_rxportconf.ring_size = 512;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   }
+   }
+   dev_info->default_rxportconf.burst_size = 32;
+   dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[dpdk-dev] [PATCH v6 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton 
Acked-by: Shreyansh Jain 
---
 app/test-pmd/cmdline.c| 31 +---
 app/test-pmd/parameters.c | 38 ++-
 app/test-pmd/testpmd.c|  5 +++--
 doc/guides/testpmd_app_ug/run_app.rst |  4 +++-
 4 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad..0914425 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
__attribute__((unused)) void *data)
 {
struct cmd_config_burst *res = parsed_result;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
}
 
if (!strcmp(res->name, "burst")) {
-   if (res->value < 1 || res->value > MAX_PKT_BURST) {
+   if (res->value == 0) {
+   /* If user gives a value of zero, query the PMD for
+* its recommended Rx burst size. Testpmd uses a single
+* size for all ports, so assume all ports are the same
+* NIC model and use the values from Port 0.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info.default_rxportconf.burst_size;
+
+   if (rec_nb_pkts == 0) {
+   printf("PMD does not recommend a burst size.\n"
+   "User provided value must be between"
+   " 1 and %d\n", MAX_PKT_BURST);
+   return;
+   } else if (rec_nb_pkts > MAX_PKT_BURST) {
+   printf("PMD recommended burst size of %d"
+   " exceeds maximum value of %d\n",
+   rec_nb_pkts, MAX_PKT_BURST);
+   return;
+   }
+   printf("Using PMD-provided burst value of %d\n",
+   rec_nb_pkts);
+   nb_pkt_per_burst = rec_nb_pkts;
+   } else if (res->value > MAX_PKT_BURST) {
printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
return;
-   }
-   nb_pkt_per_burst = res->value;
+   } else
+   nb_pkt_per_burst = res->value;
} else {
printf("Unknown parameter\n");
return;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 2192bdc..cb6a229 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
/* Default offloads for all ports. */
uint64_t rx_offloads = rx_mode.offloads;
uint64_t tx_offloads = tx_mode.offloads;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
static struct option lgopts[] = {
{ "help",   0, 0, 0 },
@@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
}
if (!strcmp(lgopts[opt_idx].name, "burst")) {
n = atoi(optarg);
-   if ((n >= 1) && (n <= MAX_PKT_BURST))
-   nb_pkt_per_burst = (uint16_t) n;
-   else
+   if (n == 0) {
+   /* A burst size of zero means that the
+* PMD should be queried for
+* recommended Rx burst size. Since
+* testpmd uses a single size for all
+* ports, port 0 is queried for the
+* value, on the assumption that all
+* ports are of the same NIC model.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info
+   

[dpdk-dev] [PATCH v6 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
Reviewed-by: Ferruh Yigit 
---
 drivers/net/e1000/em_ethdev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f727382..2035ea8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1100,6 +1100,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
ETH_LINK_SPEED_1G;
+
+   /* Preferred queue parameters */
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_txportconf.ring_size = 256;
+   dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[dpdk-dev] [PATCH v6 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

The optimal values of several transmission & reception related parameters,
such as burst sizes, descriptor ring sizes, and number of queues, varies
between different network interface devices. This patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This patchset includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.

Changes in v6:
* Updated/corrected testpmd documentation
* Carried forward acks/review
* Rebased to d218a4d060de

Changes in v5:
* uint_16_t corrected to uint16_t

Changes in v4:
* Added API/ABI change documentation
* Rebased to 78f5a2e93d74

Changes in v3:
* Changed formatting around new rte_eth_dev_info fields
* Added Doxygen documentation to struct rte_eth_dev_portconf
* Testpmd "port config all burst 0" and --burst=0 uses PMD 
  Rx burst recommendations.
* Added to release notes
* Rebased to 8ea081f38161

Changes in v2:
* Rebased to master
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/cmdline.c | 31 +---
 app/test-pmd/parameters.c  | 38 +
 app/test-pmd/testpmd.c |  5 ++--
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst | 35 +++
 doc/guides/testpmd_app_ug/run_app.rst  |  4 +++-
 drivers/net/e1000/em_ethdev.c  |  6 +
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 10 files changed, 198 insertions(+), 36 deletions(-)

-- 
2.9.5

[dpdk-dev] [PATCH v6 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
Reviewed-by: Ferruh Yigit 
Acked-by: Shreyansh Jain 
---
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst | 35 +++
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 4 files changed, 95 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst 
b/doc/guides/rel_notes/deprecation.rst
index ec70b5f..d13077d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -112,19 +112,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t 
rx_ring``,
-and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/doc/guides/rel_notes/release_18_05.rst 
b/doc/guides/rel_notes/release_18_05.rst
index abc1c17..0d57a63 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -58,6 +58,11 @@ New Features
   * Added support for NVGRE, VXLAN and GENEVE filters in flow API.
   * Added support for DROP action in flow API.
 
+* **Added PMD-recommended Tx and Rx parameters**
+
+  Applications can now query drivers for device-tuned values of
+  ring sizes, burst sizes, and number of queues.
+
 
 API Changes
 ---
@@ -83,6 +88,29 @@ API Changes
   memory footprint which helps in better cache utilization when large number
   of meter objects are used.
 
+* **Changes to semantics of rte_eth_dev_configure() parameters.**
+
+   If both the ``nb_rx_q`` and ``nb_tx_q`` parameters are zero,
+   ``rte_eth_dev_configure`` will now use PMD-recommended queue sizes, or if
+   recommendations are not provided by the PMD the function will use ethdev
+   fall-back values. Previously setting both of the parameters to zero would
+   have resulted in ``-EINVAL`` being returned.
+
+* **Changes to semantics of rte_eth_rx_queue_setup() parameters.**
+
+   If the ``nb_rx_desc`` parameter is zero, ``rte_eth_rx_queue_setup`` will
+   now use the PMD-recommended Rx ring size, or in the case where the PMD
+   does not provide a recommendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_rx_desc`` to zero would have
+   resulted in an error.
+
+* **Changes to semantics of rte_eth_tx_queue_setup() parameters.**
+
+   If the ``nb_tx_desc`` parameter is zero, ``rte_eth_tx_queue_setup`` will
+   now use the PMD-recommended Tx ring size, or in the case where the PMD
+   does not provide a recoomendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_tx_desc`` to zero would have
+   resulted in an error.
 
 ABI Changes
 ---
@@ -97,6 +125,13 @@ ABI Changes
Also, make sure to start the actual text at the margin.
=
 
+* **Additional fields in rte_eth_dev_info.**
+
+  The ``rte_eth_dev_info`` structure has had two extra entries appended to the
+  end of it: ``default_rxportconf`` and ``default_txportconf``. Each of these
+  in turn are ``rte_eth_dev_portconf`` structures containing three fields of
+  type ``uint16_t``: ``burst_size``, ``ring_size``, and ``nb_queues``. These
+  are parameter values recommended for use by the PMD.
 
 Removed Items
 -
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 2c74f7e..209796d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1061,6 +1061,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 
nb_rx_q, uint16_t nb_tx_q,
 
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EI

Re: [dpdk-dev] [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 10/04/2018 05:18, Shreyansh Jain wrote:
[..]

I'll add in the snippet above. 18.05 integration deadline has I think
passed, but documentation changes will still get in.


@thomas you won't be integrating these default conf patches in 18.05?



I meant that since the v5 and v6 will only differ in terms of docs, the 
additional changes should get in..

Re: [dpdk-dev] [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 09/04/2018 13:55, Shreyansh Jain wrote:
[..]

Documentation for burst mode changes to testpmd would need an update.
I guess, only when the user explicitly sets 'set burst 0' would the
driver defaults be picked up - isn't it?


Yes.



Maybe something like this:

--->8---
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -372,7 +372,9 @@ The commandline options are:
 *   ``--burst=N``

 Set the number of packets per burst to N, where 1 <= N <= 512.
-The default value is 16.
+The default value is 32.
+If set to 0, driver default is used if defined. Else, if driver
default
+is not defined, default of 32 is used.

 *   ``--mbcache=N``
--->8---

In the above, I think the existing documented default value needs to be
changed. It is set to '#define DEF_PKT_BURST 32'


Had a quick look and it looks like that discrepancy has been there since 
the documentation was converted to .rst in 2014.




If you add that, please use my ack for next revision.
(For patch 1/4, I had already given my Ack in v2)


I'll add in the snippet above. 18.05 integration deadline has I think 
passed, but documentation changes will still get in.

[dpdk-dev] [PATCH v5 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst | 35 +++
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 4 files changed, 95 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst 
b/doc/guides/rel_notes/deprecation.rst
index ec70b5f..d13077d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -112,19 +112,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t 
rx_ring``,
-and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/doc/guides/rel_notes/release_18_05.rst 
b/doc/guides/rel_notes/release_18_05.rst
index e5fac1c..2fe9c70 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -58,6 +58,11 @@ New Features
   * Added support for NVGRE, VXLAN and GENEVE filters in flow API.
   * Added support for DROP action in flow API.
 
+* **Added PMD-recommended Tx and Rx parameters**
+
+  Applications can now query drivers for device-tuned values of
+  ring sizes, burst sizes, and number of queues.
+
 
 API Changes
 ---
@@ -72,6 +77,29 @@ API Changes
Also, make sure to start the actual text at the margin.
=
 
+* **Changes to semantics of rte_eth_dev_configure() parameters.**
+
+   If both the ``nb_rx_q`` and ``nb_tx_q`` parameters are zero,
+   ``rte_eth_dev_configure`` will now use PMD-recommended queue sizes, or if
+   recommendations are not provided by the PMD the function will use ethdev
+   fall-back values. Previously setting both of the parameters to zero would
+   have resulted in ``-EINVAL`` being returned.
+
+* **Changes to semantics of rte_eth_rx_queue_setup() parameters.**
+
+   If the ``nb_rx_desc`` parameter is zero, ``rte_eth_rx_queue_setup`` will
+   now use the PMD-recommended Rx ring size, or in the case where the PMD
+   does not provide a recommendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_rx_desc`` to zero would have
+   resulted in an error.
+
+* **Changes to semantics of rte_eth_tx_queue_setup() parameters.**
+
+   If the ``nb_tx_desc`` parameter is zero, ``rte_eth_tx_queue_setup`` will
+   now use the PMD-recommended Tx ring size, or in the case where the PMD
+   does not provide a recoomendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_tx_desc`` to zero would have
+   resulted in an error.
 
 ABI Changes
 ---
@@ -86,6 +114,13 @@ ABI Changes
Also, make sure to start the actual text at the margin.
=
 
+* **Additional fields in rte_eth_dev_info.**
+
+  The ``rte_eth_dev_info`` structure has had two extra entries appended to the
+  end of it: ``default_rxportconf`` and ``default_txportconf``. Each of these
+  in turn are ``rte_eth_dev_portconf`` structures containing three fields of
+  type ``uint16_t``: ``burst_size``, ``ring_size``, and ``nb_queues``. These
+  are parameter values recommended for use by the PMD.
 
 Removed Items
 -
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 2c74f7e..209796d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1061,6 +1061,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 
nb_rx_q, uint16_t nb_tx_q,
 
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+   dev = &rte_eth

[dpdk-dev] [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton 
---
 app/test-pmd/cmdline.c| 31 ---
 app/test-pmd/parameters.c | 38 +-
 app/test-pmd/testpmd.c|  5 +++--
 3 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad..0914425 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
__attribute__((unused)) void *data)
 {
struct cmd_config_burst *res = parsed_result;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
}
 
if (!strcmp(res->name, "burst")) {
-   if (res->value < 1 || res->value > MAX_PKT_BURST) {
+   if (res->value == 0) {
+   /* If user gives a value of zero, query the PMD for
+* its recommended Rx burst size. Testpmd uses a single
+* size for all ports, so assume all ports are the same
+* NIC model and use the values from Port 0.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info.default_rxportconf.burst_size;
+
+   if (rec_nb_pkts == 0) {
+   printf("PMD does not recommend a burst size.\n"
+   "User provided value must be between"
+   " 1 and %d\n", MAX_PKT_BURST);
+   return;
+   } else if (rec_nb_pkts > MAX_PKT_BURST) {
+   printf("PMD recommended burst size of %d"
+   " exceeds maximum value of %d\n",
+   rec_nb_pkts, MAX_PKT_BURST);
+   return;
+   }
+   printf("Using PMD-provided burst value of %d\n",
+   rec_nb_pkts);
+   nb_pkt_per_burst = rec_nb_pkts;
+   } else if (res->value > MAX_PKT_BURST) {
printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
return;
-   }
-   nb_pkt_per_burst = res->value;
+   } else
+   nb_pkt_per_burst = res->value;
} else {
printf("Unknown parameter\n");
return;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 2192bdc..cb6a229 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
/* Default offloads for all ports. */
uint64_t rx_offloads = rx_mode.offloads;
uint64_t tx_offloads = tx_mode.offloads;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
static struct option lgopts[] = {
{ "help",   0, 0, 0 },
@@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
}
if (!strcmp(lgopts[opt_idx].name, "burst")) {
n = atoi(optarg);
-   if ((n >= 1) && (n <= MAX_PKT_BURST))
-   nb_pkt_per_burst = (uint16_t) n;
-   else
+   if (n == 0) {
+   /* A burst size of zero means that the
+* PMD should be queried for
+* recommended Rx burst size. Since
+* testpmd uses a single size for all
+* ports, port 0 is queried for the
+* value, on the assumption that all
+* ports are of the same NIC model.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info
+   .default_rxportconf.burst_size;
+
+

[dpdk-dev] [PATCH v5 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/e1000/em_ethdev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f727382..2035ea8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1100,6 +1100,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
ETH_LINK_SPEED_1G;
+
+   /* Preferred queue parameters */
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_txportconf.ring_size = 256;
+   dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[dpdk-dev] [PATCH v5 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0bf4e3..acf1f99 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3291,15 +3291,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->max_tx_queues += dev_info->vmdq_queue_num;
}
 
-   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
/* For XL710 */
dev_info->speed_capa = ETH_LINK_SPEED_40G;
-   else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+   dev_info->default_rxportconf.nb_queues = 2;
+   dev_info->default_txportconf.nb_queues = 2;
+   if (dev->data->nb_rx_queues == 1)
+   dev_info->default_rxportconf.ring_size = 2048;
+   else
+   dev_info->default_rxportconf.ring_size = 1024;
+   if (dev->data->nb_tx_queues == 1)
+   dev_info->default_txportconf.ring_size = 1024;
+   else
+   dev_info->default_txportconf.ring_size = 512;
+
+   } else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
/* For XXV710 */
dev_info->speed_capa = ETH_LINK_SPEED_25G;
-   else
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
/* For X710 */
dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+   dev_info->default_rxportconf.ring_size = 512;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   }
+   }
+   dev_info->default_rxportconf.burst_size = 32;
+   dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[dpdk-dev] [PATCH v5 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

The optimal values of several transmission & reception related parameters,
such as burst sizes, descriptor ring sizes, and number of queues, varies
between different network interface devices. This patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This patchset includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.

Changes in v5:
* uint_16_t corrected to uint16_t

Changes in v4:
* Added API/ABI change documentation
* Rebased to 78f5a2e93d74

Changes in v3:
* Changed formatting around new rte_eth_dev_info fields
* Added Doxygen documentation to struct rte_eth_dev_portconf
* Testpmd "port config all burst 0" and --burst=0 uses PMD 
  Rx burst recommendations.
* Added to release notes
* Rebased to 8ea081f38161

Changes in v2:
* Rebased to master
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/cmdline.c | 31 +---
 app/test-pmd/parameters.c  | 38 +
 app/test-pmd/testpmd.c |  5 ++--
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst | 35 +++
 drivers/net/e1000/em_ethdev.c  |  6 +
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 9 files changed, 195 insertions(+), 35 deletions(-)

-- 
2.9.5

[dpdk-dev] [PATCH v4 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton 
---
 app/test-pmd/cmdline.c| 31 ---
 app/test-pmd/parameters.c | 38 +-
 app/test-pmd/testpmd.c|  5 +++--
 3 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad..0914425 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
__attribute__((unused)) void *data)
 {
struct cmd_config_burst *res = parsed_result;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
}
 
if (!strcmp(res->name, "burst")) {
-   if (res->value < 1 || res->value > MAX_PKT_BURST) {
+   if (res->value == 0) {
+   /* If user gives a value of zero, query the PMD for
+* its recommended Rx burst size. Testpmd uses a single
+* size for all ports, so assume all ports are the same
+* NIC model and use the values from Port 0.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info.default_rxportconf.burst_size;
+
+   if (rec_nb_pkts == 0) {
+   printf("PMD does not recommend a burst size.\n"
+   "User provided value must be between"
+   " 1 and %d\n", MAX_PKT_BURST);
+   return;
+   } else if (rec_nb_pkts > MAX_PKT_BURST) {
+   printf("PMD recommended burst size of %d"
+   " exceeds maximum value of %d\n",
+   rec_nb_pkts, MAX_PKT_BURST);
+   return;
+   }
+   printf("Using PMD-provided burst value of %d\n",
+   rec_nb_pkts);
+   nb_pkt_per_burst = rec_nb_pkts;
+   } else if (res->value > MAX_PKT_BURST) {
printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
return;
-   }
-   nb_pkt_per_burst = res->value;
+   } else
+   nb_pkt_per_burst = res->value;
} else {
printf("Unknown parameter\n");
return;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 2192bdc..cb6a229 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
/* Default offloads for all ports. */
uint64_t rx_offloads = rx_mode.offloads;
uint64_t tx_offloads = tx_mode.offloads;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
static struct option lgopts[] = {
{ "help",   0, 0, 0 },
@@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
}
if (!strcmp(lgopts[opt_idx].name, "burst")) {
n = atoi(optarg);
-   if ((n >= 1) && (n <= MAX_PKT_BURST))
-   nb_pkt_per_burst = (uint16_t) n;
-   else
+   if (n == 0) {
+   /* A burst size of zero means that the
+* PMD should be queried for
+* recommended Rx burst size. Since
+* testpmd uses a single size for all
+* ports, port 0 is queried for the
+* value, on the assumption that all
+* ports are of the same NIC model.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info
+   .default_rxportconf.burst_size;
+
+

[dpdk-dev] [PATCH v4 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0bf4e3..acf1f99 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3291,15 +3291,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->max_tx_queues += dev_info->vmdq_queue_num;
}
 
-   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
/* For XL710 */
dev_info->speed_capa = ETH_LINK_SPEED_40G;
-   else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+   dev_info->default_rxportconf.nb_queues = 2;
+   dev_info->default_txportconf.nb_queues = 2;
+   if (dev->data->nb_rx_queues == 1)
+   dev_info->default_rxportconf.ring_size = 2048;
+   else
+   dev_info->default_rxportconf.ring_size = 1024;
+   if (dev->data->nb_tx_queues == 1)
+   dev_info->default_txportconf.ring_size = 1024;
+   else
+   dev_info->default_txportconf.ring_size = 512;
+
+   } else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
/* For XXV710 */
dev_info->speed_capa = ETH_LINK_SPEED_25G;
-   else
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
/* For X710 */
dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+   dev_info->default_rxportconf.ring_size = 512;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   }
+   }
+   dev_info->default_rxportconf.burst_size = 32;
+   dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[dpdk-dev] [PATCH v4 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst | 35 +++
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 4 files changed, 95 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst 
b/doc/guides/rel_notes/deprecation.rst
index ec70b5f..d13077d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -112,19 +112,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t 
rx_ring``,
-and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/doc/guides/rel_notes/release_18_05.rst 
b/doc/guides/rel_notes/release_18_05.rst
index e5fac1c..2fe9c70 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -58,6 +58,11 @@ New Features
   * Added support for NVGRE, VXLAN and GENEVE filters in flow API.
   * Added support for DROP action in flow API.
 
+* **Added PMD-recommended Tx and Rx parameters**
+
+  Applications can now query drivers for device-tuned values of
+  ring sizes, burst sizes, and number of queues.
+
 
 API Changes
 ---
@@ -72,6 +77,29 @@ API Changes
Also, make sure to start the actual text at the margin.
=
 
+* **Changes to semantics of rte_eth_dev_configure() parameters.**
+
+   If both the ``nb_rx_q`` and ``nb_tx_q`` parameters are zero,
+   ``rte_eth_dev_configure`` will now use PMD-recommended queue sizes, or if
+   recommendations are not provided by the PMD the function will use ethdev
+   fall-back values. Previously setting both of the parameters to zero would
+   have resulted in ``-EINVAL`` being returned.
+
+* **Changes to semantics of rte_eth_rx_queue_setup() parameters.**
+
+   If the ``nb_rx_desc`` parameter is zero, ``rte_eth_rx_queue_setup`` will
+   now use the PMD-recommended Rx ring size, or in the case where the PMD
+   does not provide a recommendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_rx_desc`` to zero would have
+   resulted in an error.
+
+* **Changes to semantics of rte_eth_tx_queue_setup() parameters.**
+
+   If the ``nb_tx_desc`` parameter is zero, ``rte_eth_tx_queue_setup`` will
+   now use the PMD-recommended Tx ring size, or in the case where the PMD
+   does not provide a recoomendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_tx_desc`` to zero would have
+   resulted in an error.
 
 ABI Changes
 ---
@@ -86,6 +114,13 @@ ABI Changes
Also, make sure to start the actual text at the margin.
=
 
+* **Additional fields in rte_eth_dev_info.**
+
+  The ``rte_eth_dev_info`` structure has had two extra entries appended to the
+  end of it: ``default_rxportconf`` and ``default_txportconf``. Each of these
+  in turn are ``rte_eth_dev_portconf`` structures containing three fields of
+  type ``uint_16_t``: ``burst_size``, ``ring_size``, and ``nb_queues``. These
+  are parameter values recommended for use by the PMD.
 
 Removed Items
 -
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 2c74f7e..209796d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1061,6 +1061,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 
nb_rx_q, uint16_t nb_tx_q,
 
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+   dev = &rte_eth

[dpdk-dev] [PATCH v4 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/e1000/em_ethdev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f727382..2035ea8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1100,6 +1100,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
ETH_LINK_SPEED_1G;
+
+   /* Preferred queue parameters */
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_txportconf.ring_size = 256;
+   dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[dpdk-dev] [PATCH v4 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

The optimal values of several transmission & reception related parameters,
such as burst sizes, descriptor ring sizes, and number of queues, varies
between different network interface devices. This patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This patchset includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.

Changes in v4:
* Added API/ABI change documentation
# Rebased to 78f5a2e93d74

Changes in v3:
* Changed formatting around new rte_eth_dev_info fields
* Added Doxygen documentation to struct rte_eth_dev_portconf
* Testpmd "port config all burst 0" and --burst=0 uses PMD 
  Rx burst recommendations.
* Added to release notes
* Rebased to 8ea081f38161

Changes in v2:
* Rebased to master
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/cmdline.c | 31 +---
 app/test-pmd/parameters.c  | 38 +
 app/test-pmd/testpmd.c |  5 ++--
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst | 35 +++
 drivers/net/e1000/em_ethdev.c  |  6 +
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 9 files changed, 195 insertions(+), 35 deletions(-)

-- 
2.9.5

[dpdk-dev] [PATCH v3 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0bf4e3..acf1f99 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3291,15 +3291,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->max_tx_queues += dev_info->vmdq_queue_num;
}
 
-   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
/* For XL710 */
dev_info->speed_capa = ETH_LINK_SPEED_40G;
-   else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+   dev_info->default_rxportconf.nb_queues = 2;
+   dev_info->default_txportconf.nb_queues = 2;
+   if (dev->data->nb_rx_queues == 1)
+   dev_info->default_rxportconf.ring_size = 2048;
+   else
+   dev_info->default_rxportconf.ring_size = 1024;
+   if (dev->data->nb_tx_queues == 1)
+   dev_info->default_txportconf.ring_size = 1024;
+   else
+   dev_info->default_txportconf.ring_size = 512;
+
+   } else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
/* For XXV710 */
dev_info->speed_capa = ETH_LINK_SPEED_25G;
-   else
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
/* For X710 */
dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+   dev_info->default_rxportconf.ring_size = 512;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   }
+   }
+   dev_info->default_rxportconf.burst_size = 32;
+   dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[dpdk-dev] [PATCH v3 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/e1000/em_ethdev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f727382..2035ea8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1100,6 +1100,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
ETH_LINK_SPEED_1G;
+
+   /* Preferred queue parameters */
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_txportconf.ring_size = 256;
+   dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[dpdk-dev] [PATCH v3 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton 
---
 app/test-pmd/cmdline.c| 31 ---
 app/test-pmd/parameters.c | 38 +-
 app/test-pmd/testpmd.c|  5 +++--
 3 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad..0914425 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
__attribute__((unused)) void *data)
 {
struct cmd_config_burst *res = parsed_result;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
}
 
if (!strcmp(res->name, "burst")) {
-   if (res->value < 1 || res->value > MAX_PKT_BURST) {
+   if (res->value == 0) {
+   /* If user gives a value of zero, query the PMD for
+* its recommended Rx burst size. Testpmd uses a single
+* size for all ports, so assume all ports are the same
+* NIC model and use the values from Port 0.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info.default_rxportconf.burst_size;
+
+   if (rec_nb_pkts == 0) {
+   printf("PMD does not recommend a burst size.\n"
+   "User provided value must be between"
+   " 1 and %d\n", MAX_PKT_BURST);
+   return;
+   } else if (rec_nb_pkts > MAX_PKT_BURST) {
+   printf("PMD recommended burst size of %d"
+   " exceeds maximum value of %d\n",
+   rec_nb_pkts, MAX_PKT_BURST);
+   return;
+   }
+   printf("Using PMD-provided burst value of %d\n",
+   rec_nb_pkts);
+   nb_pkt_per_burst = rec_nb_pkts;
+   } else if (res->value > MAX_PKT_BURST) {
printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
return;
-   }
-   nb_pkt_per_burst = res->value;
+   } else
+   nb_pkt_per_burst = res->value;
} else {
printf("Unknown parameter\n");
return;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 2192bdc..cb6a229 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
/* Default offloads for all ports. */
uint64_t rx_offloads = rx_mode.offloads;
uint64_t tx_offloads = tx_mode.offloads;
+   struct rte_eth_dev_info dev_info;
+   uint16_t rec_nb_pkts;
 
static struct option lgopts[] = {
{ "help",   0, 0, 0 },
@@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
}
if (!strcmp(lgopts[opt_idx].name, "burst")) {
n = atoi(optarg);
-   if ((n >= 1) && (n <= MAX_PKT_BURST))
-   nb_pkt_per_burst = (uint16_t) n;
-   else
+   if (n == 0) {
+   /* A burst size of zero means that the
+* PMD should be queried for
+* recommended Rx burst size. Since
+* testpmd uses a single size for all
+* ports, port 0 is queried for the
+* value, on the assumption that all
+* ports are of the same NIC model.
+*/
+   rte_eth_dev_info_get(0, &dev_info);
+   rec_nb_pkts = dev_info
+   .default_rxportconf.burst_size;
+
+

[dpdk-dev] [PATCH v3 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst |  5 
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 4 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst 
b/doc/guides/rel_notes/deprecation.rst
index 0c696f7..920df6b 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -115,19 +115,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t 
rx_ring``,
-and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/doc/guides/rel_notes/release_18_05.rst 
b/doc/guides/rel_notes/release_18_05.rst
index 9cc77f8..129e874 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -58,6 +58,11 @@ New Features
   * Added support for NVGRE, VXLAN and GENEVE filters in flow API.
   * Added support for DROP action in flow API.
 
+* **Added PMD-recommended Tx and Rx parameters**
+
+  Applications can now query drivers for device-tuned values of
+  ring sizes, burst sizes, and number of queues.
+
 
 API Changes
 ---
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 2c74f7e..209796d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1061,6 +1061,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 
nb_rx_q, uint16_t nb_tx_q,
 
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+   dev = &rte_eth_devices[port_id];
+
+   RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
+   (*dev->dev_ops->dev_infos_get)(dev, &dev_info);
+
+   /* If number of queues specified by application for both Rx and Tx is
+* zero, use driver preferred values. This cannot be done individually
+* as it is valid for either Tx or Rx (but not both) to be zero.
+* If driver does not provide any preferred valued, fall back on
+* EAL defaults.
+*/
+   if (nb_rx_q == 0 && nb_tx_q == 0) {
+   nb_rx_q = dev_info.default_rxportconf.nb_queues;
+   if (nb_rx_q == 0)
+   nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
+   nb_tx_q = dev_info.default_txportconf.nb_queues;
+   if (nb_tx_q == 0)
+   nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
+   }
+
if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
RTE_PMD_DEBUG_TRACE(
"Number of RX queues requested (%u) is greater than max 
supported(%d)\n",
@@ -1075,8 +1095,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, 
uint16_t nb_tx_q,
return -EINVAL;
}
 
-   dev = &rte_eth_devices[port_id];
-
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
@@ -1106,13 +1124,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 
nb_rx_q, uint16_t nb_tx_q,
 * than the maximum number of RX and TX queues supported by the
 * configured device.
 */
-   (*dev->dev_ops->dev_infos_get)(dev, &dev_info);
-
-   if (nb_rx_q == 0 && nb_tx_q == 0) {
-   RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx queue 
cannot be 0\n", port_id);
-   return -EINVAL;
-   }
-
if (nb_rx_q > dev_info.ma

[dpdk-dev] [PATCH v3 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

The optimal values of several transmission & reception related parameters,
such as burst sizes, descriptor ring sizes, and number of queues, varies
between different network interface devices. This patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This patchset includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.

Changes in v3:
* Changed formatting around new rte_eth_dev_info fields
* Added Doxygen documentation to struct rte_eth_dev_portconf
* Testpmd "port config all burst 0" and --burst=0 uses PMD 
  Rx burst recommendations.
* Added to release notes
* Rebased to 8ea081f38161

Changes in v2:
* Rebased to master
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/cmdline.c | 31 +---
 app/test-pmd/parameters.c  | 38 +
 app/test-pmd/testpmd.c |  5 ++--
 doc/guides/rel_notes/deprecation.rst   | 13 --
 doc/guides/rel_notes/release_18_05.rst |  5 
 drivers/net/e1000/em_ethdev.c  |  6 +
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 lib/librte_ether/rte_ethdev.c  | 44 +++---
 lib/librte_ether/rte_ethdev.h  | 25 +++
 9 files changed, 165 insertions(+), 35 deletions(-)

-- 
2.9.5

Re: [dpdk-dev] [PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 28/03/2018 08:18, Shreyansh Jain wrote:
[..]

Can the change for burst size too be accommodated in this patch? I
looked through them and they might not be as trivial as the change
above - but if that is incorporated in this, it might serve as
example for other applications.


Testpmd doesn't really distinguish between Rx and TX burst sizes, having 
just a single user-specifiable nb_pkt_per_burst parameter. However 
filling it with the preferred Rx burst size seems a good balance between 
providing a usage example and amount of code changes required.

Re: [dpdk-dev] [PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 31/03/2018 01:01, Thomas Monjalon wrote:
[..]

 uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
 uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */


These doxygen comments in the middle of the code are totally useless.


Did wonder why they were there. However these lines are existing code, 
and since testpmd.c uses Doxygen tags extensively, removing them in my 
view ought to be done via a separate clean-up patch.

[dpdk-dev] [PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton 
---
 app/test-pmd/testpmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..82eb197 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per port. */
 
 /*
  * Configurable number of RX/TX ring descriptors.
+ * Defaults are supplied by drivers via ethdev.
  */
-#define RTE_TEST_RX_DESC_DEFAULT 1024
-#define RTE_TEST_TX_DESC_DEFAULT 1024
+#define RTE_TEST_RX_DESC_DEFAULT 0
+#define RTE_TEST_TX_DESC_DEFAULT 0
 uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
 uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
 
-- 
2.9.5

[dpdk-dev] [PATCH v2 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 508b417..7be00ad 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3248,15 +3248,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->max_tx_queues += dev_info->vmdq_queue_num;
}
 
-   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
/* For XL710 */
dev_info->speed_capa = ETH_LINK_SPEED_40G;
-   else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+   dev_info->default_rxportconf.nb_queues = 2;
+   dev_info->default_txportconf.nb_queues = 2;
+   if (dev->data->nb_rx_queues == 1)
+   dev_info->default_rxportconf.ring_size = 2048;
+   else
+   dev_info->default_rxportconf.ring_size = 1024;
+   if (dev->data->nb_tx_queues == 1)
+   dev_info->default_txportconf.ring_size = 1024;
+   else
+   dev_info->default_txportconf.ring_size = 512;
+
+   } else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
/* For XXV710 */
dev_info->speed_capa = ETH_LINK_SPEED_25G;
-   else
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
/* For X710 */
dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+   dev_info->default_rxportconf.ring_size = 512;
+   dev_info->default_txportconf.ring_size = 256;
+   } else {
+   dev_info->default_rxportconf.ring_size = 256;
+   dev_info->default_txportconf.ring_size = 256;
+   }
+   }
+   dev_info->default_rxportconf.burst_size = 32;
+   dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[dpdk-dev] [PATCH v2 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 doc/guides/rel_notes/deprecation.rst | 13 ---
 lib/librte_ether/rte_ethdev.c| 44 
 lib/librte_ether/rte_ethdev.h| 23 +++
 3 files changed, 58 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst 
b/doc/guides/rel_notes/deprecation.rst
index 74c18ed..803038b 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -115,19 +115,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t 
rx_ring``,
-and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0590f0c..e5fba7c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1045,6 +1045,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 
nb_rx_q, uint16_t nb_tx_q,
 
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+   dev = &rte_eth_devices[port_id];
+
+   RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
+   (*dev->dev_ops->dev_infos_get)(dev, &dev_info);
+
+   /* If number of queues specified by application for both Rx and Tx is
+* zero, use driver preferred values. This cannot be done individually
+* as it is valid for either Tx or Rx (but not both) to be zero.
+* If driver does not provide any preferred valued, fall back on
+* EAL defaults.
+*/
+   if (nb_rx_q == 0 && nb_tx_q == 0) {
+   nb_rx_q = dev_info.default_rxportconf.nb_queues;
+   if (nb_rx_q == 0)
+   nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
+   nb_tx_q = dev_info.default_txportconf.nb_queues;
+   if (nb_tx_q == 0)
+   nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
+   }
+
if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
RTE_PMD_DEBUG_TRACE(
"Number of RX queues requested (%u) is greater than max 
supported(%d)\n",
@@ -1059,8 +1079,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, 
uint16_t nb_tx_q,
return -EINVAL;
}
 
-   dev = &rte_eth_devices[port_id];
-
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
@@ -1090,13 +1108,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 
nb_rx_q, uint16_t nb_tx_q,
 * than the maximum number of RX and TX queues supported by the
 * configured device.
 */
-   (*dev->dev_ops->dev_infos_get)(dev, &dev_info);
-
-   if (nb_rx_q == 0 && nb_tx_q == 0) {
-   RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx queue 
cannot be 0\n", port_id);
-   return -EINVAL;
-   }
-
if (nb_rx_q > dev_info.max_rx_queues) {
RTE_PMD_DEBUG_TRACE("ethdev port_id=%d nb_rx_queues=%d > %d\n",
port_id, nb_rx_q, dev_info.max_rx_queues);
@@ -1461,6 +1472,14 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t 
rx_queue_id,
return -EINVAL;
}
 
+   /* Use default specified by driver, if nb_rx_desc is zero */
+   if (nb_rx_desc == 0) {
+   nb_rx_desc = dev_info.default_rxportconf.ring_size;
+   /* If driver default is also zero, fall back on EAL default */
+   if (nb_rx_desc == 0)
+   nb_rx_

[dpdk-dev] [PATCH v2 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/e1000/em_ethdev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 242375f..c715065 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1152,6 +1152,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
ETH_LINK_SPEED_1G;
+
+   /* Preferred queue parameters */
+   dev_info->default_rxportconf.nb_queues = 1;
+   dev_info->default_txportconf.nb_queues = 1;
+   dev_info->default_txportconf.ring_size = 256;
+   dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[dpdk-dev] [PATCH v2 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

The optimal values of several transmission & reception related parameters,
such as burst sizes, descriptor ring sizes, and number of queues, varies
between different network interface devices. This patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This RFC/V1 includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.


Changes in v2:
* Rebased to 
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/testpmd.c   |  5 ++--
 doc/guides/rel_notes/deprecation.rst | 13 ---
 drivers/net/e1000/em_ethdev.c|  6 +
 drivers/net/i40e/i40e_ethdev.c   | 33 ---
 lib/librte_ether/rte_ethdev.c| 44 
 lib/librte_ether/rte_ethdev.h| 23 +++
 6 files changed, 97 insertions(+), 27 deletions(-)

-- 
2.9.5

Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

On 20/03/2018 15:03, Ferruh Yigit wrote:

On 3/16/2018 3:36 PM, Remy Horton wrote:

[..]


struct rte_eth_dev_preferred_size {
uint16_t burst;
uint16_t ring;
uint16_t nb_queues;
};
struct rte_eth_dev_info {
/* ... */
struct rte_eth_dev_preferred_size preferred_rx;
struct rte_eth_dev_preferred_size preferred_tx;
};


Hi Remy,

There are already two members in "struct rte_eth_dev_info":
"struct rte_eth_rxconf default_rxconf;"
"struct rte_eth_txconf default_txconf;"

These two are filled by PMDs. I think we can say these are PMD preferred values
for rte_eth_[rt]xconf structs.

Right now we are extending the preferred values that PMDs can provide.

So what about using same naming convention to be consistent with existing usage?
Something like "struct rte_eth_portconf default_portconf"?



Would default_[rt]xportconf be ok?

I would consider adding the parameters to rte_eth_[rt]xconf rather than 
creating a new rte_eth_portconf but since the former is used elsewhere 
this might cause complications.

Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

On 16/03/2018 13:54, Shreyansh Jain wrote:

On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit  wrote:

On 3/15/2018 2:39 PM, Bruce Richardson wrote:

On Thu, Mar 15, 2018 at 01:57:13PM +, Ferruh Yigit wrote:

[..]

Hi Remy, Shreyansh,

What do you think about using a variable name consistent with existing
"default_[rt]xconf" in dev_info?


It just turned out to be much more complex than I initially thought :)
Is this what the above conversation merging at (for Rx, as example):

1. 'default_rx_size_conf' is added in rte_eth_dev_info (and this
includes I/O  params like burst size, besides configure time nb_queue,
nb_desc etc). Driver would return these values filled in when
info_get() is called.


At the moment thinking of the names below, based in what I've read so far..

struct rte_eth_dev_preferred_size {
uint16_t rx_burst;
uint16_t tx_burst;
uint16_t rx_ring;
uint16_t tx_ring;
uint16_t rx_nb_queues;
uint16_t tx_nb_queues;
};
struct rte_eth_dev_info {
/* ... */
struct rte_eth_dev_preferred_size preferred_size;
};

Since Rx and Tx use the same parameters, a possible alternative is 
below, although such separation of Rx & Tx was not something I was 
planning on doing:


struct rte_eth_dev_preferred_size {
uint16_t burst;
uint16_t ring;
uint16_t nb_queues;
};
struct rte_eth_dev_info {
/* ... */
struct rte_eth_dev_preferred_size preferred_rx;
struct rte_eth_dev_preferred_size preferred_tx;
};



2a. If an application needs the defaults, it would perform info_get()
and get the values. then, use the values in configuration APIs
(rx_queue_setup for nb_rx_desc, eth_dev_dev_configure for
nb_rx_queues).
For rx_burst calls, it would use the burst_size fields obtained from info_get().
This is good enough for configuration and datapath (rx_burst).


There was also the suggestion of adding a completely new API function 
rather than using info_get() although there might be some resistance as 
struct eth_dev_ops is already pretty large.




OR, another case

2b. Application wants to use default vaules provided by driver without
calling info_get. In which case, it would call
rx_queue_setup(nb_rx_desc=0..) or eth_dev_configure(nb_rx_queue=0,
nb_tx_queue=0). The implementation would query the value from
'default_rx_size_conf' through info_get() and use those values.
Though, in this case, rte_eth_rx_burst(burst=0) might not work for
picking up the default within rte_ethdev.h.


Since rte_eth_*_burst() are fast-path functions, they are places I would 
prefer not to put conditionals.




:Four observations:
A). For burst size (or any other I/O time value added in future),
values would have to be explicitly used by application - always. If
value reported by info_get() is '0' (see (B) below), application to
use its own judgement. No default override by lib_eal.
IMO, This is good enough assumption.

B). '0' as an indicator for 'no-default-value-available-from-driver'
is still an open point. It is good enough for current proposed
parameters, but may be a valid numerical value in future.
IMO, this can be ignored for now.

C) Unlike the original proposal, this would add two separate members
to rte_eth_dev_info - one each for Rx and Tx. They both are still
expected to be populated through the info_get() implementation but not
by lib_eal.
IMO, doesn't matter.


There's been quite a bit of discussion whether ethdev should provide 
fall-back values if the PMD doesn't. In this case applications can 
assume the value is always valid and it makes the 0-as-indicator issue 
disappear, but it comes with its own set of issues.




D) Would there be no non-Rx and non-Tx defaults which need to be shared?
I am not sure about this, though.


I can't think of any off-hand.



Sorry if I am repeating everything again, but I got lost in the
conversation and needed to break it again.

Re: [dpdk-dev] [PATCH v2 3/5] net/ixgbe: add module EEPROM callbacks for ixgbe

[Remy Horton]

Did quick test and seems OK to me.

On 16/03/2018 09:42, Zijie Pan wrote:

Add new callbacks for eth_dev_ops of ixgbe to get the information
and data of plugin module eeprom.

Signed-off-by: Zijie Pan 
---
 drivers/net/ixgbe/ixgbe_ethdev.c |   79 ++
 1 file changed, 79 insertions(+)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v2 1/5] ethdev: add access to eeprom

[Remy Horton]

On 16/03/2018 09:38, Zijie Pan wrote:
[..]

Signed-off-by: Zijie Pan 
---
 doc/guides/nics/features.rst   |   11 +++
 lib/librte_ether/rte_dev_info.h|   18 ++
 lib/librte_ether/rte_ethdev.c  |   26 ++
 lib/librte_ether/rte_ethdev.h  |   35 +++
 lib/librte_ether/rte_ethdev_core.h |   12 
 5 files changed, 102 insertions(+)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH v2 2/5] examples/ethtool: add a new command module-eeprom

[Remy Horton]

On 16/03/2018 09:41, Zijie Pan wrote:
[..]

Signed-off-by: Zijie Pan 
---
 doc/guides/sample_app_ug/ethtool.rst  |2 ++
 examples/ethtool/ethtool-app/ethapp.c |   64 +
 examples/ethtool/lib/rte_ethtool.c|   30 
 examples/ethtool/lib/rte_ethtool.h|   34 ++
 4 files changed, 130 insertions(+)


Acked-by: Remy Horton 

Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

On 14/03/2018 15:10, Shreyansh Jain wrote:
[..]

What do you think renaming structure and variable name,
"preferred_dev_config" perhaps?


I missed this naming while reading this patch. In the deprecation
notice, 'preferred_size' was the name we came up with precisely on
this issue of structure having queue length and burst size.

What about using that same name?


Because of namespace issues it will need at least the rte_ prefix. 
Otherwise seems good to me as I cannot think of anything else that is 
relatively short.

Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

On 14/03/2018 14:43, Ferruh Yigit wrote:
[..]

 lib/librte_ether/rte_ethdev.c | 18 ++
 lib/librte_ether/rte_ethdev.h | 15 +++


Can you please remove deprecation notice in this patch.


Done.


+   /* Defaults for drivers that don't implement preferred
+* queue parameters.

[..]

Not sure about having these defaults here. It is OK to have defaults in driver,
in application or in config file, but I am not sure if putting them into device
abstraction layer hides them.

What about not providing any default in ethdev layer, and get zero as invalid
when using them?


This is something I have been thinking about, and I am going to remove 
them for the V2. Original motive was to avoid breaking testpmd for PMDs 
that don't give defaults (i.e. almost all of them). The alternative is 
to put place-holders into all the PMDs themselves, but I am not sure if 
this is appropriate.

Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Remy Horton]

On 14/03/2018 12:28, Shreyansh Jain wrote:
[..]

+   /* Use default specified by driver, if nb_rc_desc is zero */

^^^
 Should be '_rx_'

[..]

Other than the above;


Ok - v2 on the way :)



Acked-by: Shreyansh Jain 

Re: [dpdk-dev] [PATCH 0/5] Patches to get the information and data of EEPROM

[Remy Horton]

On 12/03/2018 04:17, Zijie Pan wrote:

Add APIs to read information from the DPDK applictions.
It can be used to dump the EEPROM of plugin modules (SFP+, QSFP, etc.).


Quite a few checkpatch warnings regarding long lines, but did a quick 
test with ixgbe and on the whole it looks good to me. CC the 
ixgbe/e1000/i40e maintainers on any v2 patchset, as they might catch 
something I've missed.

Re: [dpdk-dev] [PATCH 1/5] ethdev: add access to eeprom

[Remy Horton]

On 12/03/2018 04:17, Zijie Pan wrote:
[..]

Signed-off-by: Zijie Pan 
---
 doc/guides/nics/features.rst   |   11 +++
 lib/librte_ether/rte_dev_info.h|   18 ++
 lib/librte_ether/rte_ethdev.c  |   26 ++
 lib/librte_ether/rte_ethdev.h  |   35 +++
 lib/librte_ether/rte_ethdev_core.h |   12 
 5 files changed, 102 insertions(+)


Acked-by: Remy Horton 

Re: [dpdk-dev] [PATCH 2/5] examples/ethtool: add a new command module-eeprom

[Remy Horton]

Comments inline. On the whole looks good to me.

On 12/03/2018 04:17, Zijie Pan wrote:
[..]

Signed-off-by: Zijie Pan 
---
 doc/guides/sample_app_ug/ethtool.rst  |2 ++
 examples/ethtool/ethtool-app/ethapp.c |   61 +
 examples/ethtool/lib/rte_ethtool.c|   30 
 examples/ethtool/lib/rte_ethtool.h|   34 ++
 4 files changed, 127 insertions(+)


Reviewed-by: Remy Horton 



 cmdline_parse_token_string_t pcmd_eeprom_token_cmd =
TOKEN_STRING_INITIALIZER(struct pcmd_intstr_params, cmd, "eeprom");
+cmdline_parse_token_string_t pcmd_module_eeprom_token_cmd =
+   TOKEN_STRING_INITIALIZER(struct pcmd_intstr_params, cmd, 
"module-eeprom");


Checkpatch: 80 character limit.



+   info_eeprom.len = module_info[1];
+   info_eeprom.offset = 0;
+   stat = rte_ethtool_get_module_eeprom(params->port,
+&info_eeprom, bytes_eeprom);
+   if (stat != 0 || module_info[1] <= 0) {


This supposed to be module_info[1] rather than info_eeprom.len ?

Re: [dpdk-dev] [PATCH 3/5] net/ixgbe: add module EEPROM callbacks for ixgbe

[Remy Horton]

On 12/03/2018 04:17, Zijie Pan wrote:
[..]

Signed-off-by: Zijie Pan 
---
 drivers/net/ixgbe/ixgbe_ethdev.c |   76 ++
 1 file changed, 76 insertions(+)


Reviewed-by: Remy Horton 



+   if (addr_mode & IXGBE_SFF_ADDRESSING_MODE) {
+   PMD_DRV_LOG(ERR, "Address change required to access page 0xA2, but 
not supported. Please report the module type to the driver maintainers.");


Checkpatch: 80 characters

[dpdk-dev] [RFC PATCH v1 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton 
---
 drivers/net/i40e/i40e_ethdev.c | 35 ---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 508b417..4bcd05e 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3168,6 +3168,7 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
struct i40e_vsi *vsi = pf->main_vsi;
struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
+   struct rte_eth_dev_pref_queue_info *pref_q_info;
 
dev_info->pci_dev = pci_dev;
dev_info->max_rx_queues = vsi->nb_qps;
@@ -3248,15 +3249,43 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->max_tx_queues += dev_info->vmdq_queue_num;
}
 
-   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+   pref_q_info = &dev_info->preferred_queue_values;
+   if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
/* For XL710 */
dev_info->speed_capa = ETH_LINK_SPEED_40G;
-   else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+   pref_q_info->nb_tx_queues = 2;
+   pref_q_info->nb_rx_queues = 2;
+   if (dev->data->nb_rx_queues == 1)
+   pref_q_info->rx_ring_size = 2048;
+   else
+   pref_q_info->rx_ring_size = 1024;
+   if (dev->data->nb_tx_queues == 1)
+   pref_q_info->tx_ring_size = 1024;
+   else
+   pref_q_info->tx_ring_size = 512;
+
+   } else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
/* For XXV710 */
dev_info->speed_capa = ETH_LINK_SPEED_25G;
-   else
+   pref_q_info->nb_tx_queues = 1;
+   pref_q_info->nb_rx_queues = 1;
+   pref_q_info->rx_ring_size = 256;
+   pref_q_info->tx_ring_size = 256;
+   } else {
/* For X710 */
dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+   pref_q_info->nb_tx_queues = 1;
+   pref_q_info->nb_rx_queues = 1;
+   if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+   pref_q_info->rx_ring_size = 512;
+   pref_q_info->tx_ring_size = 256;
+   } else {
+   pref_q_info->rx_ring_size = 256;
+   pref_q_info->tx_ring_size = 256;
+   }
+   }
+   pref_q_info->tx_burst_size = 32;
+   pref_q_info->rx_burst_size = 32;
 }
 
 static int
-- 
2.9.5