DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] lib/cmdline_rdline: increase command line buf size
@ 2020-02-20 13:18 Wisam Jaddo
  2020-02-20 13:42 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  2020-02-20 14:53 ` [dpdk-dev] [PATCH v3] cmdline: increase maximum line length Wisam Jaddo
  0 siblings, 2 replies; 8+ messages in thread
From: Wisam Jaddo @ 2020-02-20 13:18 UTC (permalink / raw)
  To: dev, rasland, thomasm; +Cc: stable

The current size of buffer is not enough to fit all allowed items/actions,
thus it will block a lot of testing.

Cc: stable@dpdk.org

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
---
 lib/librte_cmdline/cmdline_rdline.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
index d217029..8193e1d 100644
--- a/lib/librte_cmdline/cmdline_rdline.h
+++ b/lib/librte_cmdline/cmdline_rdline.h
@@ -39,7 +39,7 @@ extern "C" {
 #endif
 
 /* configuration */
-#define RDLINE_BUF_SIZE 512
+#define RDLINE_BUF_SIZE 2048
 #define RDLINE_PROMPT_SIZE  32
 #define RDLINE_VT100_BUF_SIZE  8
 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] lib/cmdline_rdline: increase command line buf size
  2020-02-20 13:18 [dpdk-dev] [PATCH] lib/cmdline_rdline: increase command line buf size Wisam Jaddo
@ 2020-02-20 13:42 ` Thomas Monjalon
  2020-02-20 14:06   ` Wisam Monther
  2020-02-20 14:53 ` [dpdk-dev] [PATCH v3] cmdline: increase maximum line length Wisam Jaddo
  1 sibling, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2020-02-20 13:42 UTC (permalink / raw)
  To: Wisam Jaddo; +Cc: dev, rasland, stable, olivier.matz, bernard.iremonger

Hi,

About the title, I suggest:
cmdline: increase maximum line length

20/02/2020 14:18, Wisam Jaddo:
> The current size of buffer is not enough to fit all allowed items/actions,
> thus it will block a lot of testing.
> 
> Cc: stable@dpdk.org

+Cc maintainers of cmdline and testpmd

> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
[...]
> -#define RDLINE_BUF_SIZE 512
> +#define RDLINE_BUF_SIZE 2048

I feel 2k is reasonable.
What is the consequence on memory usage?

How critical is this change?
Which kind of command is so long?



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] lib/cmdline_rdline: increase command line buf size
  2020-02-20 13:42 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2020-02-20 14:06   ` Wisam Monther
  0 siblings, 0 replies; 8+ messages in thread
From: Wisam Monther @ 2020-02-20 14:06 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dev, Raslan Darawsheh, stable, olivier.matz, bernard.iremonger

Hi,

Sure I'll handle the title with the right prefix.

- consequence on memory usage?
I don't think it will affect much,
Since we are reading from the same buffer all the time,
Read -> reset -> read again -> ...etc

- How critical is this change?
For now it's not that critical, but for testing perspective I think it's critical,
And it's a must to have so we can check un touched scenarios.
Example:
one rule can have outer items, inner items, actions ={ header rewrite, meta actions, duplicate actions, fate action, ..etc} 
thus to test such combination we need to parse more in the cmdline

- Which kind of command is so long?
RTE_FLOW Example:
flow create 2 ingress priority 0 group 4 transfer pattern eth / ipv4 tos spec 0x0 ttl spec 0x0 proto spec 0x6 src spec 4.4.4.11 dst spec 4.4.4.10 tos mask 0x0 ttl mask 0x0 proto mask 0x0 src mask 255.255.255.255 dst mask 255.255.255.255 / tcp src spec 5001 dst spec 38392 flags spec 0x0 src mask 65535 dst mask 65535 flags mask 0x5 / tag index is 0 data spec 0x00000200 data mask 0x0000ff00 / end actions count / set_tag index 0 data 0x00000200 mask 0x0000ff00 / set_tag index 0 data 0x0000002a mask 0x000000ff / set_meta data 0x00000001 mask 0x0000ffff / jump group 7 / end

BRs,
Wisam Jaddo

-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net> 
Sent: Thursday, February 20, 2020 3:43 PM
To: Wisam Monther <wisamm@mellanox.com>
Cc: dev@dpdk.org; Raslan Darawsheh <rasland@mellanox.com>; stable@dpdk.org; olivier.matz@6wind.com; bernard.iremonger@intel.com
Subject: Re: [dpdk-stable] [PATCH] lib/cmdline_rdline: increase command line buf size

Hi,

About the title, I suggest:
cmdline: increase maximum line length

20/02/2020 14:18, Wisam Jaddo:
> The current size of buffer is not enough to fit all allowed 
> items/actions, thus it will block a lot of testing.
> 
> Cc: stable@dpdk.org

+Cc maintainers of cmdline and testpmd

> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
[...]
> -#define RDLINE_BUF_SIZE 512
> +#define RDLINE_BUF_SIZE 2048

I feel 2k is reasonable.
What is the consequence on memory usage?

How critical is this change?
Which kind of command is so long?



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v3] cmdline: increase maximum line length
  2020-02-20 13:18 [dpdk-dev] [PATCH] lib/cmdline_rdline: increase command line buf size Wisam Jaddo
  2020-02-20 13:42 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2020-02-20 14:53 ` Wisam Jaddo
  2020-02-22 15:28   ` David Marchand
  1 sibling, 1 reply; 8+ messages in thread
From: Wisam Jaddo @ 2020-02-20 14:53 UTC (permalink / raw)
  To: dev, rasland, thomas; +Cc: olivier.matz, bernard.iremonger, stable

This increase due to the usage of cmdline in dpdk applications
as config commands such as testpmd do for rte_flow rules creation.

The current size of buffer is not enough to fill
many cases of rte_flow commands validation/creation.

rte_flow now can have outer items, inner items, modify
actions, meta data actions, duplicate action, fate action and
more in one single rte flow, thus 512 char will not be enough
to validate such rte flow rules.

Such change shouldn't affect the memory since the cmdline
reading again using the same buffer.

Cc: stable@dpdk.org

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>

---
changes in v3
* Fix commit title
	Previous title was not that clear of what the change is
* Add more clarification for what we need such increase
* Explain why it won't cause any memory issue
---
---
 lib/librte_cmdline/cmdline_rdline.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h
index d217029..8193e1d 100644
--- a/lib/librte_cmdline/cmdline_rdline.h
+++ b/lib/librte_cmdline/cmdline_rdline.h
@@ -39,7 +39,7 @@ extern "C" {
 #endif
 
 /* configuration */
-#define RDLINE_BUF_SIZE 512
+#define RDLINE_BUF_SIZE 2048
 #define RDLINE_PROMPT_SIZE  32
 #define RDLINE_VT100_BUF_SIZE  8
 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] cmdline: increase maximum line length
  2020-02-20 14:53 ` [dpdk-dev] [PATCH v3] cmdline: increase maximum line length Wisam Jaddo
@ 2020-02-22 15:28   ` David Marchand
  2020-07-31 12:55     ` Olivier Matz
  0 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2020-02-22 15:28 UTC (permalink / raw)
  To: Wisam Jaddo
  Cc: dev, Raslan, Thomas Monjalon, Olivier Matz, Iremonger, Bernard,
	dpdk stable

This patch is flagged as an ABI breakage:
https://travis-ci.com/ovsrobot/dpdk/jobs/289313318#L2273


On Thu, Feb 20, 2020 at 3:53 PM Wisam Jaddo <wisamm@mellanox.com> wrote:
>
> This increase due to the usage of cmdline in dpdk applications
> as config commands such as testpmd do for rte_flow rules creation.
>
> The current size of buffer is not enough to fill
> many cases of rte_flow commands validation/creation.
>
> rte_flow now can have outer items, inner items, modify
> actions, meta data actions, duplicate action, fate action and
> more in one single rte flow, thus 512 char will not be enough
> to validate such rte flow rules.
>
> Such change shouldn't affect the memory since the cmdline
> reading again using the same buffer.

I don't get your point here.


> Cc: stable@dpdk.org

This is not a fix.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] cmdline: increase maximum line length
  2020-02-22 15:28   ` David Marchand
@ 2020-07-31 12:55     ` Olivier Matz
  2020-07-31 13:00       ` David Marchand
  2020-07-31 15:46       ` Stephen Hemminger
  0 siblings, 2 replies; 8+ messages in thread
From: Olivier Matz @ 2020-07-31 12:55 UTC (permalink / raw)
  To: David Marchand
  Cc: Wisam Jaddo, dev, Raslan, Thomas Monjalon, Iremonger, Bernard,
	dpdk stable

Hi,

Ressurecting this old thread.

On Sat, Feb 22, 2020 at 04:28:15PM +0100, David Marchand wrote:
> This patch is flagged as an ABI breakage:
> https://travis-ci.com/ovsrobot/dpdk/jobs/289313318#L2273
> 

In case we want this fix for 20.11, should we do a deprecation notice
in 20.08?


Olivier


> 
> On Thu, Feb 20, 2020 at 3:53 PM Wisam Jaddo <wisamm@mellanox.com> wrote:
> >
> > This increase due to the usage of cmdline in dpdk applications
> > as config commands such as testpmd do for rte_flow rules creation.
> >
> > The current size of buffer is not enough to fill
> > many cases of rte_flow commands validation/creation.
> >
> > rte_flow now can have outer items, inner items, modify
> > actions, meta data actions, duplicate action, fate action and
> > more in one single rte flow, thus 512 char will not be enough
> > to validate such rte flow rules.
> >
> > Such change shouldn't affect the memory since the cmdline
> > reading again using the same buffer.
> 
> I don't get your point here.
> 
> 
> > Cc: stable@dpdk.org
> 
> This is not a fix.
> 
> 
> -- 
> David Marchand
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] cmdline: increase maximum line length
  2020-07-31 12:55     ` Olivier Matz
@ 2020-07-31 13:00       ` David Marchand
  2020-07-31 15:46       ` Stephen Hemminger
  1 sibling, 0 replies; 8+ messages in thread
From: David Marchand @ 2020-07-31 13:00 UTC (permalink / raw)
  To: Olivier Matz
  Cc: Wisam Jaddo, dev, Raslan, Thomas Monjalon, Iremonger, Bernard,
	dpdk stable

On Fri, Jul 31, 2020 at 2:55 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> Ressurecting this old thread.
>
> On Sat, Feb 22, 2020 at 04:28:15PM +0100, David Marchand wrote:
> > This patch is flagged as an ABI breakage:
> > https://travis-ci.com/ovsrobot/dpdk/jobs/289313318#L2273
> >
>
> In case we want this fix for 20.11, should we do a deprecation notice
> in 20.08?

If there is something to change, that would be removing this max size
rather than extend it.
Let's not go the "XX bytes ought to be enough for anybody" way.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] cmdline: increase maximum line length
  2020-07-31 12:55     ` Olivier Matz
  2020-07-31 13:00       ` David Marchand
@ 2020-07-31 15:46       ` Stephen Hemminger
  1 sibling, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2020-07-31 15:46 UTC (permalink / raw)
  To: Olivier Matz
  Cc: David Marchand, Wisam Jaddo, dev, Raslan, Thomas Monjalon,
	Iremonger, Bernard, dpdk stable

On Fri, 31 Jul 2020 14:55:16 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:

> Hi,
> 
> Ressurecting this old thread.
> 
> On Sat, Feb 22, 2020 at 04:28:15PM +0100, David Marchand wrote:
> > This patch is flagged as an ABI breakage:
> > https://travis-ci.com/ovsrobot/dpdk/jobs/289313318#L2273
> >   
> 
> In case we want this fix for 20.11, should we do a deprecation notice
> in 20.08?
> 
> 
> Olivier
> 
> 
> > 
> > On Thu, Feb 20, 2020 at 3:53 PM Wisam Jaddo <wisamm@mellanox.com> wrote:  
> > >
> > > This increase due to the usage of cmdline in dpdk applications
> > > as config commands such as testpmd do for rte_flow rules creation.
> > >
> > > The current size of buffer is not enough to fill
> > > many cases of rte_flow commands validation/creation.
> > >
> > > rte_flow now can have outer items, inner items, modify
> > > actions, meta data actions, duplicate action, fate action and
> > > more in one single rte flow, thus 512 char will not be enough
> > > to validate such rte flow rules.
> > >
> > > Such change shouldn't affect the memory since the cmdline
> > > reading again using the same buffer.  
> > 
> > I don't get your point here.

The cmdline is a awkward user API. Thomas wanted to replace it but
it seems to have gotten nowhere.

Agree that having something dynamic would be best, Something
based of getline() or editline (readline).

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-31 15:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20 13:18 [dpdk-dev] [PATCH] lib/cmdline_rdline: increase command line buf size Wisam Jaddo
2020-02-20 13:42 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2020-02-20 14:06   ` Wisam Monther
2020-02-20 14:53 ` [dpdk-dev] [PATCH v3] cmdline: increase maximum line length Wisam Jaddo
2020-02-22 15:28   ` David Marchand
2020-07-31 12:55     ` Olivier Matz
2020-07-31 13:00       ` David Marchand
2020-07-31 15:46       ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).