[PATCH 0/3] ABI changes (RETA, cmdline)

[PATCH 0/3] ABI changes (RETA, cmdline)

[Nelio Laranjeiro]

Change ABIs as announced:

 * rte_eth_rss_reta_entry64.reta field to handle large number of
   queues (above 256).

 * Increase cmdline buffers to support long command lines, include long
   RETA update commands.

Nelio Laranjeiro (3):
  ethdev: change RETA type in rte_eth_rss_reta_entry64
  cmdline: increase command line buffer
  mlx5: increase RETA table size

 app/test-pmd/cmdline.c                    |  4 ++--
 doc/guides/rel_notes/deprecation.rst      | 10 ----------
 drivers/net/mlx5/mlx5_defs.h              |  2 +-
 lib/librte_cmdline/cmdline_parse.h        |  2 +-
 lib/librte_cmdline/cmdline_parse_string.h |  2 +-
 lib/librte_cmdline/cmdline_rdline.h       |  2 +-
 lib/librte_ether/rte_ethdev.c             |  2 +-
 lib/librte_ether/rte_ethdev.h             |  2 +-
 8 files changed, 8 insertions(+), 18 deletions(-)

-- 
2.1.4

[PATCH 1/3] ethdev: change RETA type in rte_eth_rss_reta_entry64

[Nelio Laranjeiro]

Several NICs can handle 512 entries/queues in their RETA table, an 8 bit field
is not large enough for them.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 app/test-pmd/cmdline.c               | 4 ++--
 doc/guides/rel_notes/deprecation.rst | 5 -----
 lib/librte_ether/rte_ethdev.c        | 2 +-
 lib/librte_ether/rte_ethdev.h        | 2 +-
 4 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..9c7cda0 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1767,7 +1767,7 @@ parse_reta_config(const char *str,
 	int i;
 	unsigned size;
 	uint16_t hash_index, idx, shift;
-	uint8_t nb_queue;
+	uint16_t nb_queue;
 	char s[256];
 	const char *p, *p0 = str;
 	char *end;
@@ -1800,7 +1800,7 @@ parse_reta_config(const char *str,
 		}
 
 		hash_index = (uint16_t)int_fld[FLD_HASH_INDEX];
-		nb_queue = (uint8_t)int_fld[FLD_QUEUE];
+		nb_queue = (uint16_t)int_fld[FLD_QUEUE];
 
 		if (hash_index >= nb_entries) {
 			printf("Invalid RETA hash index=%d\n", hash_index);
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e94d4a2..e8a3ed6 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -15,11 +15,6 @@ Deprecation Notices
 * The ethdev structures rte_eth_link, rte_eth_dev_info and rte_eth_conf
   must be updated to support 100G link and to have a cleaner link speed API.
 
-* ABI changes is planned for the reta field in struct rte_eth_rss_reta_entry64
-  which handles at most 256 queues (8 bits) while newer NICs support larger
-  tables (512 queues).
-  It should be integrated in release 2.3.
-
 * ABI changes are planned for struct rte_eth_fdir_flow in order to support
   extend flow director's input set. The release 2.2 does not contain these ABI
   changes, but release 2.3 will, and no backwards compatibility is planned.
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index ed971b4..b0aa94d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1857,7 +1857,7 @@ rte_eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
 static int
 rte_eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
 			 uint16_t reta_size,
-			 uint8_t max_rxq)
+			 uint16_t max_rxq)
 {
 	uint16_t i, idx, shift;
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index bada8ad..8302a2d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -520,7 +520,7 @@ struct rte_eth_mirror_conf {
 struct rte_eth_rss_reta_entry64 {
 	uint64_t mask;
 	/**< Mask bits indicate which entries need to be updated/queried. */
-	uint8_t reta[RTE_RETA_GROUP_SIZE];
+	uint16_t reta[RTE_RETA_GROUP_SIZE];
 	/**< Group of 64 redirection table entries. */
 };
 
-- 
2.1.4

[PATCH 2/3] cmdline: increase command line buffer

[Nelio Laranjeiro]

For RETA query/update with a table of 512 entries, buffers are too small to
handle the request.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 doc/guides/rel_notes/deprecation.rst      | 5 -----
 lib/librte_cmdline/cmdline_parse.h        | 2 +-
 lib/librte_cmdline/cmdline_parse_string.h | 2 +-
 lib/librte_cmdline/cmdline_rdline.h       | 2 +-
 4 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e8a3ed6..9930b5a 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -39,8 +39,3 @@ Deprecation Notices
   and table action handlers will be updated:
   the pipeline parameter will be added, the packets mask parameter will be
   either removed (for input port action handler) or made input-only.
-
-* ABI changes are planned in cmdline buffer size to allow the use of long
-  commands (such as RETA update in testpmd).  This should impact
-  CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE.
-  It should be integrated in release 2.3.
diff --git a/lib/librte_cmdline/cmdline_parse.h b/lib/librte_cmdline/cmdline_parse.h
index 4b25c45..89b28b1 100644
--- a/lib/librte_cmdline/cmdline_parse.h
+++ b/lib/librte_cmdline/cmdline_parse.h
@@ -81,7 +81,7 @@ extern "C" {
 #define CMDLINE_PARSE_COMPLETED_BUFFER  2
 
 /* maximum buffer size for parsed result */
-#define CMDLINE_PARSE_RESULT_BUFSIZE 8192
+#define CMDLINE_PARSE_RESULT_BUFSIZE 65536
 
 /**
  * Stores a pointer to the ops struct, and the offset: the place to
diff --git a/lib/librte_cmdline/cmdline_parse_string.h b/lib/librte_cmdline/cmdline_parse_string.h
index c205622..61e0627 100644
--- a/lib/librte_cmdline/cmdline_parse_string.h
+++ b/lib/librte_cmdline/cmdline_parse_string.h
@@ -66,7 +66,7 @@ extern "C" {
 #endif
 
 /* size of a parsed string */
-#define STR_TOKEN_SIZE 128
+#define STR_TOKEN_SIZE 8192
 
 typedef char cmdline_fixed_string_t[STR_TOKEN_SIZE];
 
diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
index b9aad9b..07f8faa 100644
--- a/lib/librte_cmdline/cmdline_rdline.h
+++ b/lib/librte_cmdline/cmdline_rdline.h
@@ -93,7 +93,7 @@ extern "C" {
 #endif
 
 /* configuration */
-#define RDLINE_BUF_SIZE 256
+#define RDLINE_BUF_SIZE 16384
 #define RDLINE_PROMPT_SIZE  32
 #define RDLINE_VT100_BUF_SIZE  8
 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
-- 
2.1.4

[PATCH 3/3] mlx5: increase RETA table size

[Nelio Laranjeiro]

ConnectX-4 NICs can handle at most 512 entries in RETA table.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_defs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index bb82c9a..ae5eda9 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -47,7 +47,7 @@
 #define MLX5_PMD_TX_PER_COMP_REQ 64
 
 /* RSS Indirection table size. */
-#define RSS_INDIRECTION_TABLE_SIZE 128
+#define RSS_INDIRECTION_TABLE_SIZE 512
 
 /* Maximum number of Scatter/Gather Elements per Work Request. */
 #ifndef MLX5_PMD_SGE_WR_N
-- 
2.1.4

[PATCH v2 0/3] ABI change for RETA, cmdline

[Nelio Laranjeiro]

Previous version of commit
"cmdline: increase command line buffer", had side effects and was breaking
some commands.

In this version, I only applied John McNamara's solution which consists in
increasing only RDLINE_BUF_SIZE define from 256 to 512 bytes [1].

[1] http://dpdk.org/ml/archives/dev/2015-November/027643.html

Nelio Laranjeiro (3):
  cmdline: increase command line buffer
  ethdev: change RETA type in rte_eth_rss_reta_entry64
  mlx5: increase RETA table size

 app/test-pmd/cmdline.c               |  4 ++--
 doc/guides/rel_notes/deprecation.rst | 10 ----------
 drivers/net/mlx5/mlx5_defs.h         |  2 +-
 lib/librte_cmdline/cmdline_rdline.h  |  2 +-
 lib/librte_ether/rte_ethdev.c        |  2 +-
 lib/librte_ether/rte_ethdev.h        |  2 +-
 6 files changed, 6 insertions(+), 16 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] cmdline: increase command line buffer

[Nelio Laranjeiro]

Allow long command lines in testpmd (like flow director with IPv6, ...).

Signed-off-by: John McNamara <john.mcnamara@intel.com>
Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 -----
 lib/librte_cmdline/cmdline_rdline.h  | 2 +-
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e94d4a2..9cb288c 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -44,8 +44,3 @@ Deprecation Notices
   and table action handlers will be updated:
   the pipeline parameter will be added, the packets mask parameter will be
   either removed (for input port action handler) or made input-only.
-
-* ABI changes are planned in cmdline buffer size to allow the use of long
-  commands (such as RETA update in testpmd).  This should impact
-  CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE.
-  It should be integrated in release 2.3.
diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
index b9aad9b..72e2dad 100644
--- a/lib/librte_cmdline/cmdline_rdline.h
+++ b/lib/librte_cmdline/cmdline_rdline.h
@@ -93,7 +93,7 @@ extern "C" {
 #endif
 
 /* configuration */
-#define RDLINE_BUF_SIZE 256
+#define RDLINE_BUF_SIZE 512
 #define RDLINE_PROMPT_SIZE  32
 #define RDLINE_VT100_BUF_SIZE  8
 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
-- 
2.1.4

Re: [PATCH v2 1/3] cmdline: increase command line buffer

[Panu Matilainen]

On 01/12/2016 12:49 PM, Nelio Laranjeiro wrote:
> Allow long command lines in testpmd (like flow director with IPv6, ...).
>
> Signed-off-by: John McNamara <john.mcnamara@intel.com>
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>   doc/guides/rel_notes/deprecation.rst | 5 -----
>   lib/librte_cmdline/cmdline_rdline.h  | 2 +-
>   2 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index e94d4a2..9cb288c 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -44,8 +44,3 @@ Deprecation Notices
>     and table action handlers will be updated:
>     the pipeline parameter will be added, the packets mask parameter will be
>     either removed (for input port action handler) or made input-only.
> -
> -* ABI changes are planned in cmdline buffer size to allow the use of long
> -  commands (such as RETA update in testpmd).  This should impact
> -  CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE.
> -  It should be integrated in release 2.3.
> diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
> index b9aad9b..72e2dad 100644
> --- a/lib/librte_cmdline/cmdline_rdline.h
> +++ b/lib/librte_cmdline/cmdline_rdline.h
> @@ -93,7 +93,7 @@ extern "C" {
>   #endif
>
>   /* configuration */
> -#define RDLINE_BUF_SIZE 256
> +#define RDLINE_BUF_SIZE 512
>   #define RDLINE_PROMPT_SIZE  32
>   #define RDLINE_VT100_BUF_SIZE  8
>   #define RDLINE_HISTORY_BUF_SIZE BUFSIZ

Having to break a library ABI for a change like this is a bit ridiculous.

I didn't try it so could be wrong, but based on a quick look, struct 
rdline could easily be made opaque to consumers by just adding functions 
for allocating and freeing it.

	- Panu -

Re: [PATCH v2 1/3] cmdline: increase command line buffer

[Nélio Laranjeiro]

On Tue, Jan 12, 2016 at 02:46:07PM +0200, Panu Matilainen wrote:
> On 01/12/2016 12:49 PM, Nelio Laranjeiro wrote:
> >Allow long command lines in testpmd (like flow director with IPv6, ...).
> >
> >Signed-off-by: John McNamara <john.mcnamara@intel.com>
> >Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> >---
> >  doc/guides/rel_notes/deprecation.rst | 5 -----
> >  lib/librte_cmdline/cmdline_rdline.h  | 2 +-
> >  2 files changed, 1 insertion(+), 6 deletions(-)
> >
> >diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> >index e94d4a2..9cb288c 100644
> >--- a/doc/guides/rel_notes/deprecation.rst
> >+++ b/doc/guides/rel_notes/deprecation.rst
> >@@ -44,8 +44,3 @@ Deprecation Notices
> >    and table action handlers will be updated:
> >    the pipeline parameter will be added, the packets mask parameter will be
> >    either removed (for input port action handler) or made input-only.
> >-
> >-* ABI changes are planned in cmdline buffer size to allow the use of long
> >-  commands (such as RETA update in testpmd).  This should impact
> >-  CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE.
> >-  It should be integrated in release 2.3.
> >diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
> >index b9aad9b..72e2dad 100644
> >--- a/lib/librte_cmdline/cmdline_rdline.h
> >+++ b/lib/librte_cmdline/cmdline_rdline.h
> >@@ -93,7 +93,7 @@ extern "C" {
> >  #endif
> >
> >  /* configuration */
> >-#define RDLINE_BUF_SIZE 256
> >+#define RDLINE_BUF_SIZE 512
> >  #define RDLINE_PROMPT_SIZE  32
> >  #define RDLINE_VT100_BUF_SIZE  8
> >  #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
> 
> Having to break a library ABI for a change like this is a bit ridiculous.

Sure, but John McNamara needed it to handle flow director with IPv6[1].

For my part, I was needing it to manipulate the RETA table, but as I
wrote in the cover letter, it ends by breaking other commands.
Olivier Matz, has proposed another way to handle long commands lines[2],
it could be a good idea to go on this direction.

For RETA situation, we already discussed on a new API, but for now, I
do not have time for it (and as it is another ABI breakage it could only
be done for 16.07 or 2.4)[3].

If this patch is no more needed we can just drop it, for that I would
like to have the point of view from John.

> 
> I didn't try it so could be wrong, but based on a quick look, struct rdline
> could easily be made opaque to consumers by just adding functions for
> allocating and freeing it.
> 
> 	- Panu -
> 

[1] http://dpdk.org/ml/archives/dev/2015-November/027643.html
[2] http://dpdk.org/ml/archives/dev/2015-November/028557.html
[3] http://dpdk.org/ml/archives/dev/2015-October/025294.html

-- 
Nélio Laranjeiro
6WIND

Re: [PATCH v2 1/3] cmdline: increase command line buffer

[Panu Matilainen]

On 01/15/2016 10:44 AM, Nélio Laranjeiro wrote:
> On Tue, Jan 12, 2016 at 02:46:07PM +0200, Panu Matilainen wrote:
>> On 01/12/2016 12:49 PM, Nelio Laranjeiro wrote:
>>> Allow long command lines in testpmd (like flow director with IPv6, ...).
>>>
>>> Signed-off-by: John McNamara <john.mcnamara@intel.com>
>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>> ---
>>>   doc/guides/rel_notes/deprecation.rst | 5 -----
>>>   lib/librte_cmdline/cmdline_rdline.h  | 2 +-
>>>   2 files changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>> index e94d4a2..9cb288c 100644
>>> --- a/doc/guides/rel_notes/deprecation.rst
>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>> @@ -44,8 +44,3 @@ Deprecation Notices
>>>     and table action handlers will be updated:
>>>     the pipeline parameter will be added, the packets mask parameter will be
>>>     either removed (for input port action handler) or made input-only.
>>> -
>>> -* ABI changes are planned in cmdline buffer size to allow the use of long
>>> -  commands (such as RETA update in testpmd).  This should impact
>>> -  CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE.
>>> -  It should be integrated in release 2.3.
>>> diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
>>> index b9aad9b..72e2dad 100644
>>> --- a/lib/librte_cmdline/cmdline_rdline.h
>>> +++ b/lib/librte_cmdline/cmdline_rdline.h
>>> @@ -93,7 +93,7 @@ extern "C" {
>>>   #endif
>>>
>>>   /* configuration */
>>> -#define RDLINE_BUF_SIZE 256
>>> +#define RDLINE_BUF_SIZE 512
>>>   #define RDLINE_PROMPT_SIZE  32
>>>   #define RDLINE_VT100_BUF_SIZE  8
>>>   #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
>>
>> Having to break a library ABI for a change like this is a bit ridiculous.
>
> Sure, but John McNamara needed it to handle flow director with IPv6[1].
>
> For my part, I was needing it to manipulate the RETA table, but as I
> wrote in the cover letter, it ends by breaking other commands.
> Olivier Matz, has proposed another way to handle long commands lines[2],
> it could be a good idea to go on this direction.
>
> For RETA situation, we already discussed on a new API, but for now, I
> do not have time for it (and as it is another ABI breakage it could only
> be done for 16.07 or 2.4)[3].
>
> If this patch is no more needed we can just drop it, for that I would
> like to have the point of view from John.

Note that I was not objecting to the patch as such, I can easily see 256 
characters not being enough for commandline buffer.

I was merely noting that having to break an ABI to increase an 
effectively internal buffer size is a sign of a, um, less-than-optimal 
library design.

Apologies if I wasn't clear about that,

	- Panu -

Re: [PATCH v2 1/3] cmdline: increase command line buffer

[Olivier MATZ]

Hi,

On 01/15/2016 10:00 AM, Panu Matilainen wrote:
>>>> diff --git a/lib/librte_cmdline/cmdline_rdline.h
>>>> b/lib/librte_cmdline/cmdline_rdline.h
>>>> index b9aad9b..72e2dad 100644
>>>> --- a/lib/librte_cmdline/cmdline_rdline.h
>>>> +++ b/lib/librte_cmdline/cmdline_rdline.h
>>>> @@ -93,7 +93,7 @@ extern "C" {
>>>>   #endif
>>>>
>>>>   /* configuration */
>>>> -#define RDLINE_BUF_SIZE 256
>>>> +#define RDLINE_BUF_SIZE 512
>>>>   #define RDLINE_PROMPT_SIZE  32
>>>>   #define RDLINE_VT100_BUF_SIZE  8
>>>>   #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
>>>
>>> Having to break a library ABI for a change like this is a bit
>>> ridiculous.
>>
>> Sure, but John McNamara needed it to handle flow director with IPv6[1].
>>
>> For my part, I was needing it to manipulate the RETA table, but as I
>> wrote in the cover letter, it ends by breaking other commands.
>> Olivier Matz, has proposed another way to handle long commands lines[2],
>> it could be a good idea to go on this direction.
>>
>> For RETA situation, we already discussed on a new API, but for now, I
>> do not have time for it (and as it is another ABI breakage it could only
>> be done for 16.07 or 2.4)[3].
>>
>> If this patch is no more needed we can just drop it, for that I would
>> like to have the point of view from John.
> 
> Note that I was not objecting to the patch as such, I can easily see 256
> characters not being enough for commandline buffer.
> 
> I was merely noting that having to break an ABI to increase an
> effectively internal buffer size is a sign of a, um, less-than-optimal
> library design.

You are right about the cmdline ABI. Changing this buffer size
should not imply an ABI change. I'll try to find some time to
investigate this issue.

Another question we could raise is: should we export the API of
librte_cmdline to external applications? Now that baremetal dpdk is
not supported, having this library in dpdk is probably useless as
we can surely find standard replacements for it. A first step could
be to mark it as "internal".

About the patch Nélio's patch itself, I'm not so convinced having more
than 256 characters is absolutely required, and I would prefer to see
the commands beeing reworked to be more human-readable. On the other
hand, the ABI breakage was announced so there is no reason to nack
this patch now.

Regards,
Olivier

Re: [PATCH v2 1/3] cmdline: increase command line buffer

[Nélio Laranjeiro]

On Mon, Jan 18, 2016 at 03:38:43PM +0100, Olivier MATZ wrote:
> Hi,
> 
> On 01/15/2016 10:00 AM, Panu Matilainen wrote:
> >>>> diff --git a/lib/librte_cmdline/cmdline_rdline.h
> >>>> b/lib/librte_cmdline/cmdline_rdline.h
> >>>> index b9aad9b..72e2dad 100644
> >>>> --- a/lib/librte_cmdline/cmdline_rdline.h
> >>>> +++ b/lib/librte_cmdline/cmdline_rdline.h
> >>>> @@ -93,7 +93,7 @@ extern "C" {
> >>>>   #endif
> >>>>
> >>>>   /* configuration */
> >>>> -#define RDLINE_BUF_SIZE 256
> >>>> +#define RDLINE_BUF_SIZE 512
> >>>>   #define RDLINE_PROMPT_SIZE  32
> >>>>   #define RDLINE_VT100_BUF_SIZE  8
> >>>>   #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
> >>>
> >>> Having to break a library ABI for a change like this is a bit
> >>> ridiculous.
> >>
> >> Sure, but John McNamara needed it to handle flow director with IPv6[1].
> >>
> >> For my part, I was needing it to manipulate the RETA table, but as I
> >> wrote in the cover letter, it ends by breaking other commands.
> >> Olivier Matz, has proposed another way to handle long commands lines[2],
> >> it could be a good idea to go on this direction.
> >>
> >> For RETA situation, we already discussed on a new API, but for now, I
> >> do not have time for it (and as it is another ABI breakage it could only
> >> be done for 16.07 or 2.4)[3].
> >>
> >> If this patch is no more needed we can just drop it, for that I would
> >> like to have the point of view from John.
> > 
> > Note that I was not objecting to the patch as such, I can easily see 256
> > characters not being enough for commandline buffer.
> > 
> > I was merely noting that having to break an ABI to increase an
> > effectively internal buffer size is a sign of a, um, less-than-optimal
> > library design.
> 
> You are right about the cmdline ABI. Changing this buffer size
> should not imply an ABI change. I'll try to find some time to
> investigate this issue.
> 
> Another question we could raise is: should we export the API of
> librte_cmdline to external applications? Now that baremetal dpdk is
> not supported, having this library in dpdk is probably useless as
> we can surely find standard replacements for it. A first step could
> be to mark it as "internal".
> 
> About the patch Nélio's patch itself, I'm not so convinced having more
> than 256 characters is absolutely required, and I would prefer to see
> the commands beeing reworked to be more human-readable. On the other
> hand, the ABI breakage was announced so there is no reason to nack
> this patch now.
> 
> Regards,
> Olivier

John,

What is your position about this patch?
Is it still needed?

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [PATCH v2 1/3] cmdline: increase command line buffer

[Mcnamara, John]

> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Friday, February 26, 2016 3:17 PM
> To: Mcnamara, John <john.mcnamara@intel.com>
> Cc: Olivier MATZ <olivier.matz@6wind.com>; Panu Matilainen
> <pmatilai@redhat.com>; dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>;
> thomas.monjalon@6wind.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> olgas@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v2 1/3] cmdline: increase command line
> buffer
> 
> 
> What is your position about this patch?
> Is it still needed?

Yes. It is still required.

Acked-by: John McNamara <john.mcnamara@intel.com>

[PATCH v2 2/3] ethdev: change RETA type in rte_eth_rss_reta_entry64

[Nelio Laranjeiro]

Several NICs can handle 512 entries/queues in their RETA table, an 8 bit field
is not large enough for them.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 app/test-pmd/cmdline.c               | 4 ++--
 doc/guides/rel_notes/deprecation.rst | 5 -----
 lib/librte_ether/rte_ethdev.c        | 2 +-
 lib/librte_ether/rte_ethdev.h        | 2 +-
 4 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..9c7cda0 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1767,7 +1767,7 @@ parse_reta_config(const char *str,
 	int i;
 	unsigned size;
 	uint16_t hash_index, idx, shift;
-	uint8_t nb_queue;
+	uint16_t nb_queue;
 	char s[256];
 	const char *p, *p0 = str;
 	char *end;
@@ -1800,7 +1800,7 @@ parse_reta_config(const char *str,
 		}
 
 		hash_index = (uint16_t)int_fld[FLD_HASH_INDEX];
-		nb_queue = (uint8_t)int_fld[FLD_QUEUE];
+		nb_queue = (uint16_t)int_fld[FLD_QUEUE];
 
 		if (hash_index >= nb_entries) {
 			printf("Invalid RETA hash index=%d\n", hash_index);
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 9cb288c..9930b5a 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -15,11 +15,6 @@ Deprecation Notices
 * The ethdev structures rte_eth_link, rte_eth_dev_info and rte_eth_conf
   must be updated to support 100G link and to have a cleaner link speed API.
 
-* ABI changes is planned for the reta field in struct rte_eth_rss_reta_entry64
-  which handles at most 256 queues (8 bits) while newer NICs support larger
-  tables (512 queues).
-  It should be integrated in release 2.3.
-
 * ABI changes are planned for struct rte_eth_fdir_flow in order to support
   extend flow director's input set. The release 2.2 does not contain these ABI
   changes, but release 2.3 will, and no backwards compatibility is planned.
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index ed971b4..b0aa94d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1857,7 +1857,7 @@ rte_eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
 static int
 rte_eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
 			 uint16_t reta_size,
-			 uint8_t max_rxq)
+			 uint16_t max_rxq)
 {
 	uint16_t i, idx, shift;
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index bada8ad..8302a2d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -520,7 +520,7 @@ struct rte_eth_mirror_conf {
 struct rte_eth_rss_reta_entry64 {
 	uint64_t mask;
 	/**< Mask bits indicate which entries need to be updated/queried. */
-	uint8_t reta[RTE_RETA_GROUP_SIZE];
+	uint16_t reta[RTE_RETA_GROUP_SIZE];
 	/**< Group of 64 redirection table entries. */
 };
 
-- 
2.1.4

[PATCH v2 3/3] mlx5: increase RETA table size

[Nelio Laranjeiro]

ConnectX-4 NICs can handle at most 512 entries in RETA table.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_defs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index bb82c9a..ae5eda9 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -47,7 +47,7 @@
 #define MLX5_PMD_TX_PER_COMP_REQ 64
 
 /* RSS Indirection table size. */
-#define RSS_INDIRECTION_TABLE_SIZE 128
+#define RSS_INDIRECTION_TABLE_SIZE 512
 
 /* Maximum number of Scatter/Gather Elements per Work Request. */
 #ifndef MLX5_PMD_SGE_WR_N
-- 
2.1.4

Re: [PATCH v2 0/3] ABI change for RETA, cmdline

[Adrien Mazarguil]

On Tue, Jan 12, 2016 at 11:49:06AM +0100, Nelio Laranjeiro wrote:
> Previous version of commit
> "cmdline: increase command line buffer", had side effects and was breaking
> some commands.
> 
> In this version, I only applied John McNamara's solution which consists in
> increasing only RDLINE_BUF_SIZE define from 256 to 512 bytes [1].
> 
> [1] http://dpdk.org/ml/archives/dev/2015-November/027643.html

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

Re: [PATCH v2 0/3] ABI change for RETA, cmdline

[Thomas Monjalon]

2016-03-03 16:16, Adrien Mazarguil:
> On Tue, Jan 12, 2016 at 11:49:06AM +0100, Nelio Laranjeiro wrote:
> > Previous version of commit
> > "cmdline: increase command line buffer", had side effects and was breaking
> > some commands.
> > 
> > In this version, I only applied John McNamara's solution which consists in
> > increasing only RDLINE_BUF_SIZE define from 256 to 512 bytes [1].
> > 
> > [1] http://dpdk.org/ml/archives/dev/2015-November/027643.html
> 
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Applied, thanks

[PATCH 0/3] Add RETA configuration to mlx5

[Adrien Mazarguil]

mlx5 devices support indirection tables of variable size up to 512 entries,
which requires a larger configuration structure (requiring a change in the
ABI).

This patchset can be considered as a first RFC step because the current API
is not very practical due to the following limitations:

- Configuration with chunks of 64 entries.
- Fixed total number of entries (previously 128, now 512).
- RETA configuration with testpmd is quite tedious (all entries must be
  specified with really long lines).

Nelio Laranjeiro (3):
  cmdline: increase command line buffer
  ethdev: change RETA type in rte_eth_rss_reta_entry64
  mlx5: RETA query/update support

 drivers/net/mlx5/mlx5.c                   |   4 +
 drivers/net/mlx5/mlx5.h                   |   7 ++
 drivers/net/mlx5/mlx5_ethdev.c            |  29 ++++++
 drivers/net/mlx5/mlx5_rss.c               | 163 ++++++++++++++++++++++++++++++
 drivers/net/mlx5/mlx5_rxq.c               |  53 ++--------
 drivers/net/mlx5/mlx5_utils.h             |  20 ++++
 lib/librte_cmdline/cmdline_parse.h        |   2 +-
 lib/librte_cmdline/cmdline_parse_string.h |   2 +-
 lib/librte_cmdline/cmdline_rdline.h       |   2 +-
 lib/librte_ether/rte_ethdev.h             |   2 +-
 10 files changed, 235 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH v2 0/3] Add RETA configuration to mlx5

[Adrien Mazarguil]

mlx5 devices support indirection tables of variable size up to 512 entries,
which requires a larger configuration structure (requiring a change in the
ABI).

This patchset can be considered as a first RFC step because the current API
is not very practical due to the following limitations:

- Configuration with chunks of 64 entries.
- Fixed total number of entries (previously 128, now 512).
- RETA configuration with testpmd is quite tedious (all entries must be
  specified with really long lines).

Changes in v2:
- None, but sending again anyway following v2 of previous patchsets
  ("Mellanox ConnectX-4 PMD (mlx5)",
  "Enhance mlx5 with Mellanox OFED 3.1" and
  "Add link status notification support to Mellanox PMDs").

Nelio Laranjeiro (3):
  cmdline: increase command line buffer
  ethdev: change RETA type in rte_eth_rss_reta_entry64
  mlx5: RETA query/update support

 drivers/net/mlx5/mlx5.c                   |   4 +
 drivers/net/mlx5/mlx5.h                   |   7 ++
 drivers/net/mlx5/mlx5_ethdev.c            |  29 ++++++
 drivers/net/mlx5/mlx5_rss.c               | 163 ++++++++++++++++++++++++++++++
 drivers/net/mlx5/mlx5_rxq.c               |  53 ++--------
 drivers/net/mlx5/mlx5_utils.h             |  20 ++++
 lib/librte_cmdline/cmdline_parse.h        |   2 +-
 lib/librte_cmdline/cmdline_parse_string.h |   2 +-
 lib/librte_cmdline/cmdline_rdline.h       |   2 +-
 lib/librte_ether/rte_ethdev.h             |   2 +-
 10 files changed, 235 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH v2 1/3] cmdline: increase command line buffer

[Adrien Mazarguil]

From: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

For RETA query/update with a table of 512 entries, buffers are too small to
handle the request.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 lib/librte_cmdline/cmdline_parse.h        | 2 +-
 lib/librte_cmdline/cmdline_parse_string.h | 2 +-
 lib/librte_cmdline/cmdline_rdline.h       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_cmdline/cmdline_parse.h b/lib/librte_cmdline/cmdline_parse.h
index 4b25c45..89b28b1 100644
--- a/lib/librte_cmdline/cmdline_parse.h
+++ b/lib/librte_cmdline/cmdline_parse.h
@@ -81,7 +81,7 @@ extern "C" {
 #define CMDLINE_PARSE_COMPLETED_BUFFER  2
 
 /* maximum buffer size for parsed result */
-#define CMDLINE_PARSE_RESULT_BUFSIZE 8192
+#define CMDLINE_PARSE_RESULT_BUFSIZE 65536
 
 /**
  * Stores a pointer to the ops struct, and the offset: the place to
diff --git a/lib/librte_cmdline/cmdline_parse_string.h b/lib/librte_cmdline/cmdline_parse_string.h
index c205622..61e0627 100644
--- a/lib/librte_cmdline/cmdline_parse_string.h
+++ b/lib/librte_cmdline/cmdline_parse_string.h
@@ -66,7 +66,7 @@ extern "C" {
 #endif
 
 /* size of a parsed string */
-#define STR_TOKEN_SIZE 128
+#define STR_TOKEN_SIZE 8192
 
 typedef char cmdline_fixed_string_t[STR_TOKEN_SIZE];
 
diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
index b9aad9b..07f8faa 100644
--- a/lib/librte_cmdline/cmdline_rdline.h
+++ b/lib/librte_cmdline/cmdline_rdline.h
@@ -93,7 +93,7 @@ extern "C" {
 #endif
 
 /* configuration */
-#define RDLINE_BUF_SIZE 256
+#define RDLINE_BUF_SIZE 16384
 #define RDLINE_PROMPT_SIZE  32
 #define RDLINE_VT100_BUF_SIZE  8
 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
-- 
2.1.0