[PATCH] cfgfile: increase value length

[PATCH] cfgfile: increase value length

[Vipin Varghese]

The default value for CFG_VALUE_LEN is set to 256 characters.
This limits the parsing for longer strings in configuration file.
Setting the default to 2048 characters increases the value array
size in `struct rte_cfgfile_entry`.

Files using cfgfile library are
1. drivers/net/mvpp2/
2. app/test-dma-perf/
3. app/test/
4. examples/qos_sched/

The structure `rte_cfgfile_entry` is not included in DPDK libraries.
Modifying from 256 to 2048 allows `app/test-dma-perf/main.c` helps to
parse longer string as shared in https://bugs.dpdk.org/show_bug.cgi?id=1333

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
 lib/cfgfile/rte_cfgfile.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/cfgfile/rte_cfgfile.h b/lib/cfgfile/rte_cfgfile.h
index 232c65c77b..401353c44e 100644
--- a/lib/cfgfile/rte_cfgfile.h
+++ b/lib/cfgfile/rte_cfgfile.h
@@ -24,7 +24,7 @@ extern "C" {
 #endif
 
 #ifndef CFG_VALUE_LEN
-#define CFG_VALUE_LEN 256
+#define CFG_VALUE_LEN 2048
 #endif
 
 /** Configuration file */
-- 
2.34.1

Re: [PATCH] cfgfile: increase value length

[Bruce Richardson]

On Wed, Dec 06, 2023 at 04:59:52PM +0530, Vipin Varghese wrote:
> The default value for CFG_VALUE_LEN is set to 256 characters.
> This limits the parsing for longer strings in configuration file.
> Setting the default to 2048 characters increases the value array
> size in `struct rte_cfgfile_entry`.
> 
> Files using cfgfile library are
> 1. drivers/net/mvpp2/
> 2. app/test-dma-perf/
> 3. app/test/
> 4. examples/qos_sched/
> 
> The structure `rte_cfgfile_entry` is not included in DPDK libraries.
> Modifying from 256 to 2048 allows `app/test-dma-perf/main.c` helps to
> parse longer string as shared in https://bugs.dpdk.org/show_bug.cgi?id=1333
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
>  lib/cfgfile/rte_cfgfile.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/cfgfile/rte_cfgfile.h b/lib/cfgfile/rte_cfgfile.h
> index 232c65c77b..401353c44e 100644
> --- a/lib/cfgfile/rte_cfgfile.h
> +++ b/lib/cfgfile/rte_cfgfile.h
> @@ -24,7 +24,7 @@ extern "C" {
>  #endif
>  
>  #ifndef CFG_VALUE_LEN
> -#define CFG_VALUE_LEN 256
> +#define CFG_VALUE_LEN 2048
>  #endif
>  
An increase seems ok to me, but is an 8x increase really necessary? If
lines in the config files are over 1k in size, then it implies that some
other mechanism would surely be better for configuration.
Can we make do with an increase to 512 only?

/Bruce

RE: [PATCH] cfgfile: increase value length

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Wednesday, December 6, 2023 1:22 PM
> To: Vipin Varghese <vipin.varghese@amd.com>
> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> Ferruh Yigit <ferruh.yigit@amd.com>
> Subject: Re: [PATCH] cfgfile: increase value length
> 
> On Wed, Dec 06, 2023 at 04:59:52PM +0530, Vipin Varghese wrote:
> > The default value for CFG_VALUE_LEN is set to 256 characters.
> > This limits the parsing for longer strings in configuration file.
> > Setting the default to 2048 characters increases the value array
> > size in `struct rte_cfgfile_entry`.
> >
> > Files using cfgfile library are
> > 1. drivers/net/mvpp2/
> > 2. app/test-dma-perf/
> > 3. app/test/
> > 4. examples/qos_sched/
> >
> > The structure `rte_cfgfile_entry` is not included in DPDK libraries.
> > Modifying from 256 to 2048 allows `app/test-dma-perf/main.c` helps to
> > parse longer string as shared in
> https://bugs.dpdk.org/show_bug.cgi?id=1333
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> > Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
> > ---
> >  lib/cfgfile/rte_cfgfile.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/cfgfile/rte_cfgfile.h b/lib/cfgfile/rte_cfgfile.h
> > index 232c65c77b..401353c44e 100644
> > --- a/lib/cfgfile/rte_cfgfile.h
> > +++ b/lib/cfgfile/rte_cfgfile.h
> > @@ -24,7 +24,7 @@ extern "C" {
> >  #endif
> >
> >  #ifndef CFG_VALUE_LEN
> > -#define CFG_VALUE_LEN 256
> > +#define CFG_VALUE_LEN 2048
> >  #endif
> >
> An increase seems ok to me, but is an 8x increase really necessary? If
> lines in the config files are over 1k in size, then it implies that some
> other mechanism would surely be better for configuration.
> Can we make do with an increase to 512 only?
> 
> /Bruce

Happy to compromise with 1K.

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Re: [PATCH] cfgfile: increase value length

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]

[AMD Official Use Only - General]

Thanks Bruce & Cristian for the comments.

An increase seems ok to me, but is an 8x increase really necessary? If
lines in the config files are over 1k in size, then it implies that some
other mechanism would surely be better for configuration.
Can we make do with an increase to 512 only?

VV> We encountered this issue https://bugs.dpdk.org/show_bug.cgi?id=1333 trying to use multiple queue with DSA. But I hear you 256 to 2048 is big jump.


Happy to compromise with 1K.
VV> Sure let me update this is v2.

Since there is a ABI breakage, https://patchwork.dpdk.org/project/dpdk/patch/20231206112952.1588-1-vipin.varghese@amd.com/ I will re work and share v2.

________________________________
From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
Sent: 06 December 2023 19:04
To: Richardson, Bruce <bruce.richardson@intel.com>; Varghese, Vipin <Vipin.Varghese@amd.com>
Cc: dev@dpdk.org <dev@dpdk.org>; Yigit, Ferruh <Ferruh.Yigit@amd.com>
Subject: RE: [PATCH] cfgfile: increase value length

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Wednesday, December 6, 2023 1:22 PM
> To: Vipin Varghese <vipin.varghese@amd.com>
> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> Ferruh Yigit <ferruh.yigit@amd.com>
> Subject: Re: [PATCH] cfgfile: increase value length
>
> On Wed, Dec 06, 2023 at 04:59:52PM +0530, Vipin Varghese wrote:
> > The default value for CFG_VALUE_LEN is set to 256 characters.
> > This limits the parsing for longer strings in configuration file.
> > Setting the default to 2048 characters increases the value array
> > size in `struct rte_cfgfile_entry`.
> >
> > Files using cfgfile library are
> > 1. drivers/net/mvpp2/
> > 2. app/test-dma-perf/
> > 3. app/test/
> > 4. examples/qos_sched/
> >
> > The structure `rte_cfgfile_entry` is not included in DPDK libraries.
> > Modifying from 256 to 2048 allows `app/test-dma-perf/main.c` helps to
> > parse longer string as shared in
> https://bugs.dpdk.org/show_bug.cgi?id=1333
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> > Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
> > ---
> >  lib/cfgfile/rte_cfgfile.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/cfgfile/rte_cfgfile.h b/lib/cfgfile/rte_cfgfile.h
> > index 232c65c77b..401353c44e 100644
> > --- a/lib/cfgfile/rte_cfgfile.h
> > +++ b/lib/cfgfile/rte_cfgfile.h
> > @@ -24,7 +24,7 @@ extern "C" {
> >  #endif
> >
> >  #ifndef CFG_VALUE_LEN
> > -#define CFG_VALUE_LEN 256
> > +#define CFG_VALUE_LEN 2048
> >  #endif
> >
> An increase seems ok to me, but is an 8x increase really necessary? If
> lines in the config files are over 1k in size, then it implies that some
> other mechanism would surely be better for configuration.
> Can we make do with an increase to 512 only?
>
> /Bruce

Happy to compromise with 1K.

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>


[-- Attachment #2: Type: text/html, Size: 8779 bytes --]

Re: [PATCH] cfgfile: increase value length

[Bruce Richardson]

On Wed, Dec 06, 2023 at 03:22:41PM +0000, Varghese, Vipin wrote:
>    [AMD Official Use Only - General]
> 
>    Thanks Bruce & Cristian for the comments.
>    An increase seems ok to me, but is an 8x increase really necessary? If
>    lines in the config files are over 1k in size, then it implies that
>    some
>    other mechanism would surely be better for configuration.
>    Can we make do with an increase to 512 only?
>    VV> We encountered this issue
>    [1]https://bugs.dpdk.org/show_bug.cgi?id=1333 trying to use multiple
>    queue with DSA. But I hear you 256 to 2048 is big jump.
>    Happy to compromise with 1K.
>    VV> Sure let me update this is v2.
>    Since there is a ABI breakage,
>    [2]https://patchwork.dpdk.org/project/dpdk/patch/20231206112952.1588-1-
>    vipin.varghese@amd.com/ I will re work and share v2.

Looking at the bugzilla, I still think even an increase to 1k is too much,
and that we should look to adjust the config file format in some way to
accomodate these longer line settings.

Taking the specific example in the bug:
* each entry in the line is wasting 5 characters with the string "lcore"
* each PCI ID is starting with 0000, so we can do like EAL does and assume
  PCI domain is "0000" if just the BDF is given. This may involve changes
  to the PCI driver in question, since i assume these queues are names
  rather than addresses, but it would make them easier to address.

If we did these two items, then:
lcore_dma=lcore10@0000:00:04.2-q0,lcore12@0000:00:04.2-q1,lcore13@0000:00:04.2-q2,lcore14@0000:00:04.2-q3,lcore15@0000:00:04.2-q4,lcore16@0000:00:04.2-q5,lcore17@0000:00:04.2-q6,lcore18@0000:00:04.2-q7,lcore19@0000:00:04.4-q0,lcore20@0000:00:04.4-q1,lcore21@0000:00:04.4-q2,lcore22@0000:00:04.4-q3,lcore23@0000:00:04.4-q4,lcore24@0000:00:04.4-q5,lcore25@0000:00:04.4-q6,lcore26@0000:00:04.4-q7

becomes:
lcore_dma=10@00:04.2-q0,12@00:04.2-q1,13@00:04.2-q2,14@00:04.2-q3,15@00:04.2-q4,16@00:04.2-q5,17@00:04.2-q6,18@00:04.2-q7,19@00:04.4-q0,20@00:04.4-q1,21@00:04.4-q2,22@00:04.4-q3,23@00:04.4-q4,24@00:04.4-q5,25@00:04.4-q6,26@00:04.4-q7

a reduction from 394 chars to 234 (10 chars per device).

However, a better alternative may be to split up the lcore_dma item
entirely, so that we always have one element per line:

lcore_dma0=lcore10@0000:04.2-q0
lcore_dma1=lcore12@0000:04.2-q1
....
lcore_dma15=lcore26@0000:04.4-q7

What is especially good about using this is a solution is that it can be fully
backward compatible and has no ABI issues. If an "lcore_dma=" line exists,
we can split it, otherwise look for lcore_dma0, lcore_dma1 etc.

As I stated above, the need for really long lines in a config file is a
symptom of a more serious config problem, rather than an issue with the
cfgfile lib itself.

/Bruce

Re: [PATCH] cfgfile: increase value length

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]

[AMD Official Use Only - General]

From: Bruce Richardson <bruce.richardson@intel.com>
Sent: 06 December 2023 21:20
To: Varghese, Vipin <Vipin.Varghese@amd.com>
Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org <dev@dpdk.org>; Yigit, Ferruh <Ferruh.Yigit@amd.com>; Parikh, Neerav <Neerav.Parikh@amd.com>
Subject: Re: [PATCH] cfgfile: increase value length

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


On Wed, Dec 06, 2023 at 03:22:41PM +0000, Varghese, Vipin wrote:
>    [AMD Official Use Only - General]
>
>    Thanks Bruce & Cristian for the comments.
>    An increase seems ok to me, but is an 8x increase really necessary? If
>    lines in the config files are over 1k in size, then it implies that
>    some
>    other mechanism would surely be better for configuration.
>    Can we make do with an increase to 512 only?
>    VV> We encountered this issue
>    [1]https://bugs.dpdk.org/show_bug.cgi?id=1333 trying to use multiple
>    queue with DSA. But I hear you 256 to 2048 is big jump.
>    Happy to compromise with 1K.
>    VV> Sure let me update this is v2.
>    Since there is a ABI breakage,
>    [2]https://patchwork.dpdk.org/project/dpdk/patch/20231206112952.1588-1-
>    vipin.varghese@amd.com/ I will re work and share v2.

Looking at the bugzilla, I still think even an increase to 1k is too much,
and that we should look to adjust the config file format in some way to
accomodate these longer line settings.

Based on my testing 32 devices with the current format 2048B are appropriate.
That 4 DSA devices with 8 queues each.

Taking the specific example in the bug:
* each entry in the line is wasting 5 characters with the string "lcore"
* each PCI ID is starting with 0000, so we can do like EAL does and assume
  PCI domain is "0000" if just the BDF is given. This may involve changes
  to the PCI driver in question, since i assume these queues are names
  rather than addresses, but it would make them easier to address.

If we did these two items, then:
lcore_dma=lcore10@0000:00:04.2-q0,lcore12@0000:00:04.2-q1,lcore13@0000:00:04.2-q2,lcore14@0000:00:04.2-q3,lcore15@0000:00:04.2-q4,lcore16@0000:00:04.2-q5,lcore17@0000:00:04.2-q6,lcore18@0000:00:04.2-q7,lcore19@0000:00:04.4-q0,lcore20@0000:00:04.4-q1,lcore21@0000:00:04.4-q2,lcore22@0000:00:04.4-q3,lcore23@0000:00:04.4-q4,lcore24@0000:00:04.4-q5,lcore25@0000:00:04.4-q6,lcore26@0000:00:04.4-q7

becomes:
lcore_dma=10@00:04.2-q0,12@00:04.2-q1,13@00:04.2-q2,14@00:04.2-q3,15@00:04.2-q4,16@00:04.2-q5,17@00:04.2-q6,18@00:04.2-q7,19@00:04.4-q0,20@00:04.4-q1,21@00:04.4-q2,22@00:04.4-q3,23@00:04.4-q4,24@00:04.4-q5,25@00:04.4-q6,26@00:04.4-q7

a reduction from 394 chars to 234 (10 chars per device).

I see what you are suggesting, even though DPDK documentation states 9. EAL parameters — Data Plane Development Kit 23.11.0 documentation (dpdk.org)<https://doc.dpdk.org/guides/linux_gsg/linux_eal_parameters.html> `<[domain:]bus:devid.func>`, omit the domain to `bus:devid.func>`. That will reduce the bytes I agree.

However, a better alternative may be to split up the lcore_dma item
entirely, so that we always have one element per line:

lcore_dma0=lcore10@0000:04.2-q0
lcore_dma1=lcore12@0000:04.2-q1
....
lcore_dma15=lcore26@0000:04.4-q7

What is especially good about using this is a solution is that it can be fully
backward compatible and has no ABI issues. If an "lcore_dma=" line exists,
we can split it, otherwise look for lcore_dma0, lcore_dma1 etc.

Ok, but this means the test-dma-perf will need parsing changes and cfgfile library.

As I stated above, the need for really long lines in a config file is a
symptom of a more serious config problem, rather than an issue with the
cfgfile lib itself.

I do not fully agree to this statement, here are my 2 points


  1.
current code is `#ifndef CFG_VALUE_LEN` which means, the user can change the value using compiler flag `-DCFG_VALUE_LEN=[user desired value]` as cflags.
  2.
If not declared as CFLAGS configuration value is assumed to be hardened to take only 256

Hence the reasoning would be `it was not assumed there would be change from 256B was not expected or explored. If 256B is the hardlimit then we should not have used #ifndef`.

But +1 for changing the parsing logic.


/Bruce

[-- Attachment #2: Type: text/html, Size: 8325 bytes --]

Re: [PATCH] cfgfile: increase value length

[David Marchand]

On Wed, Dec 6, 2023 at 12:31 PM Vipin Varghese <vipin.varghese@amd.com> wrote:
>
> The default value for CFG_VALUE_LEN is set to 256 characters.
> This limits the parsing for longer strings in configuration file.
> Setting the default to 2048 characters increases the value array
> size in `struct rte_cfgfile_entry`.
>
> Files using cfgfile library are
> 1. drivers/net/mvpp2/
> 2. app/test-dma-perf/
> 3. app/test/
> 4. examples/qos_sched/
>
> The structure `rte_cfgfile_entry` is not included in DPDK libraries.
> Modifying from 256 to 2048 allows `app/test-dma-perf/main.c` helps to
> parse longer string as shared in https://bugs.dpdk.org/show_bug.cgi?id=1333
>
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
>  lib/cfgfile/rte_cfgfile.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/cfgfile/rte_cfgfile.h b/lib/cfgfile/rte_cfgfile.h
> index 232c65c77b..401353c44e 100644
> --- a/lib/cfgfile/rte_cfgfile.h
> +++ b/lib/cfgfile/rte_cfgfile.h
> @@ -24,7 +24,7 @@ extern "C" {
>  #endif
>
>  #ifndef CFG_VALUE_LEN
> -#define CFG_VALUE_LEN 256
> +#define CFG_VALUE_LEN 2048
>  #endif
>
>  /** Configuration file */

Last time I looked at this code, I had the impression such a change
would break the ABI.

I see that the discussion stopped at a suggestion to change some parsing logic.
For now, I marked this patch as Changes requested.

Thanks.


-- 
David Marchand

Re: [PATCH] cfgfile: increase value length

[Varghese, Vipin]

On 2/1/2024 4:02 PM, David Marchand wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Dec 6, 2023 at 12:31 PM Vipin Varghese <vipin.varghese@amd.com> wrote:
>> The default value for CFG_VALUE_LEN is set to 256 characters.
>> This limits the parsing for longer strings in configuration file.
>> Setting the default to 2048 characters increases the value array
>> size in `struct rte_cfgfile_entry`.
>>
>> Files using cfgfile library are
>> 1. drivers/net/mvpp2/
>> 2. app/test-dma-perf/
>> 3. app/test/
>> 4. examples/qos_sched/
>>
>> The structure `rte_cfgfile_entry` is not included in DPDK libraries.
>> Modifying from 256 to 2048 allows `app/test-dma-perf/main.c` helps to
>> parse longer string as shared in https://bugs.dpdk.org/show_bug.cgi?id=1333
>>
>> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
>> Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
>> ---
>>   lib/cfgfile/rte_cfgfile.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/cfgfile/rte_cfgfile.h b/lib/cfgfile/rte_cfgfile.h
>> index 232c65c77b..401353c44e 100644
>> --- a/lib/cfgfile/rte_cfgfile.h
>> +++ b/lib/cfgfile/rte_cfgfile.h
>> @@ -24,7 +24,7 @@ extern "C" {
>>   #endif
>>
>>   #ifndef CFG_VALUE_LEN
>> -#define CFG_VALUE_LEN 256
>> +#define CFG_VALUE_LEN 2048
>>   #endif
>>
>>   /** Configuration file */
> Last time I looked at this code, I had the impression such a change
> would break the ABI.

Thanks David, Internally & 7 Dec 2023 based on the covnersation email 
from Bruce & Cristian; the new approach DPDK community should be taking 
is not to use comma seperated DMA instance.

Instead make DMA instances like `dma-1:, dma-2:, dma-3 ... etc`.


>
> I see that the discussion stopped at a suggestion to change some parsing logic.
> For now, I marked this patch as Changes requested.

I will make this deffer-ed or close the same.


>
> Thanks.
>
>
> --
> David Marchand
>