ethdev: support action with any config object type

Message ID 1389143e204e85c90b4fc124f9e561f43f78175e.1561989889.git.dekelp@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: support action with any config object type |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Dekel Peled July 1, 2019, 2:10 p.m. UTC
  In current implementation, an action which requires parameters
must accept them enclosed in a structure.
Some actions require a single, trivial type parameter, but it still
must be enclosed in a structure.
This obligation results in multiple, action-specific structures, each
containing a single trivial type parameter.

This patch introduces a new approach, allowing an action configuration
object of any type, trivial or a structure.

This patch introduces, in test-pmd, a new macro ARG_ENTRY_HTON, to
allow using a single argument, not enclosed in a structure.

Signed-off-by: Dekel Peled <dekelp@mellanox.com>
---
 app/test-pmd/cmdline_flow.c        |  8 ++++++++
 doc/guides/prog_guide/rte_flow.rst |  5 +++--
 lib/librte_ethdev/rte_flow.h       | 11 ++++++-----
 3 files changed, 17 insertions(+), 7 deletions(-)
  

Comments

Andrew Rybchenko July 2, 2019, 8:09 a.m. UTC | #1
On 01.07.2019 17:10, Dekel Peled wrote:
> In current implementation, an action which requires parameters
> must accept them enclosed in a structure.
> Some actions require a single, trivial type parameter, but it still
> must be enclosed in a structure.
> This obligation results in multiple, action-specific structures, each
> containing a single trivial type parameter.
>
> This patch introduces a new approach, allowing an action configuration
> object of any type, trivial or a structure.
>
> This patch introduces, in test-pmd, a new macro ARG_ENTRY_HTON, to
> allow using a single argument, not enclosed in a structure.
>
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>

The term "object" confuses me a bit, but I'm not a native speaker
so it could be just my wrong association. I'd prefer
"configuration data".

Anyway,
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
  
Dekel Peled July 2, 2019, 8:42 a.m. UTC | #2
Thanks, PSB.

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Tuesday, July 2, 2019 11:09 AM
> To: Dekel Peled <dekelp@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; wenzhuo.lu@intel.com;
> jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
> <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>; arybchenko@solarflare.com
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: support action with any config
> object type
> 
> On 01.07.2019 17:10, Dekel Peled wrote:
> > In current implementation, an action which requires parameters must
> > accept them enclosed in a structure.
> > Some actions require a single, trivial type parameter, but it still
> > must be enclosed in a structure.
> > This obligation results in multiple, action-specific structures, each
> > containing a single trivial type parameter.
> >
> > This patch introduces a new approach, allowing an action configuration
> > object of any type, trivial or a structure.
> >
> > This patch introduces, in test-pmd, a new macro ARG_ENTRY_HTON, to
> > allow using a single argument, not enclosed in a structure.
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> 
> The term "object" confuses me a bit, but I'm not a native speaker so it could
> be just my wrong association. I'd prefer "configuration data".

In previous version I wrote just "action configuration", and changed to "action configuration object" per Adrien's suggestion. I think it is better, but if it causes confusion maybe it should be changed.

Adrien, what do you think? Does "configuration data" carry the correct meaning?

> 
> Anyway,
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
  
Adrien Mazarguil July 2, 2019, 9:57 a.m. UTC | #3
On Tue, Jul 02, 2019 at 08:42:41AM +0000, Dekel Peled wrote:
> Thanks, PSB.
> 
> > -----Original Message-----
> > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > Sent: Tuesday, July 2, 2019 11:09 AM
> > To: Dekel Peled <dekelp@mellanox.com>; Adrien Mazarguil
> > <adrien.mazarguil@6wind.com>; wenzhuo.lu@intel.com;
> > jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
> > <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Slava
> > Ovsiienko <viacheslavo@mellanox.com>; arybchenko@solarflare.com
> > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: support action with any config
> > object type
> > 
> > On 01.07.2019 17:10, Dekel Peled wrote:
> > > In current implementation, an action which requires parameters must
> > > accept them enclosed in a structure.
> > > Some actions require a single, trivial type parameter, but it still
> > > must be enclosed in a structure.
> > > This obligation results in multiple, action-specific structures, each
> > > containing a single trivial type parameter.
> > >
> > > This patch introduces a new approach, allowing an action configuration
> > > object of any type, trivial or a structure.
> > >
> > > This patch introduces, in test-pmd, a new macro ARG_ENTRY_HTON, to
> > > allow using a single argument, not enclosed in a structure.
> > >
> > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > 
> > The term "object" confuses me a bit, but I'm not a native speaker so it could
> > be just my wrong association. I'd prefer "configuration data".
> 
> In previous version I wrote just "action configuration", and changed to "action configuration object" per Adrien's suggestion. I think it is better, but if it causes confusion maybe it should be changed.
> 
> Adrien, what do you think? Does "configuration data" carry the correct meaning?

Well I'm no native speaker either but "object" is the term used in the C
standard with a well-defined meaning [1] and encompasses everything
(integers, floats, structures, unions, functions, pointers, arrays):

 "region of data storage in the execution environment, the contents of which
  can represent values"

I think it's a bit less vague than "data" because whenever objects are
mentioned in the standard, they always have a type. There's no such thing as
a C object without one, and rte_flow puts a lot of emphasis on documenting
them.

 int foo;
 struct { ... } foo;
 double foo;
 char foo[];
 void *foo;
 
Whatever the type, would you refer to "foo" itself as an "object" or as
"data"?

Unrelated, but you must remove ARG_ENTRY_HTON from this patch since there's
no testpmd change in there that requires it. There's no tolerance for dead
code in testpmd as it doesn't expose an API.

Thanks.

[1] 3.14 "object"
    http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
  
Andrew Rybchenko July 2, 2019, 10:11 a.m. UTC | #4
On 02.07.2019 12:57, Adrien Mazarguil wrote:
> On Tue, Jul 02, 2019 at 08:42:41AM +0000, Dekel Peled wrote:
>> Thanks, PSB.
>>
>>> -----Original Message-----
>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Sent: Tuesday, July 2, 2019 11:09 AM
>>> To: Dekel Peled <dekelp@mellanox.com>; Adrien Mazarguil
>>> <adrien.mazarguil@6wind.com>; wenzhuo.lu@intel.com;
>>> jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
>>> <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Slava
>>> Ovsiienko <viacheslavo@mellanox.com>; arybchenko@solarflare.com
>>> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>
>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: support action with any config
>>> object type
>>>
>>> On 01.07.2019 17:10, Dekel Peled wrote:
>>>> In current implementation, an action which requires parameters must
>>>> accept them enclosed in a structure.
>>>> Some actions require a single, trivial type parameter, but it still
>>>> must be enclosed in a structure.
>>>> This obligation results in multiple, action-specific structures, each
>>>> containing a single trivial type parameter.
>>>>
>>>> This patch introduces a new approach, allowing an action configuration
>>>> object of any type, trivial or a structure.
>>>>
>>>> This patch introduces, in test-pmd, a new macro ARG_ENTRY_HTON, to
>>>> allow using a single argument, not enclosed in a structure.
>>>>
>>>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
>>> The term "object" confuses me a bit, but I'm not a native speaker so it could
>>> be just my wrong association. I'd prefer "configuration data".
>> In previous version I wrote just "action configuration", and changed to "action configuration object" per Adrien's suggestion. I think it is better, but if it causes confusion maybe it should be changed.
>>
>> Adrien, what do you think? Does "configuration data" carry the correct meaning?
> Well I'm no native speaker either but "object" is the term used in the C
> standard with a well-defined meaning [1] and encompasses everything
> (integers, floats, structures, unions, functions, pointers, arrays):
>
>   "region of data storage in the execution environment, the contents of which
>    can represent values"
>
> I think it's a bit less vague than "data" because whenever objects are
> mentioned in the standard, they always have a type. There's no such thing as
> a C object without one, and rte_flow puts a lot of emphasis on documenting
> them.
>
>   int foo;
>   struct { ... } foo;
>   double foo;
>   char foo[];
>   void *foo;
>   
> Whatever the type, would you refer to "foo" itself as an "object" or as
> "data"?

Adrien, thanks a lot. Now "object" looks OK and better than "data".

> Unrelated, but you must remove ARG_ENTRY_HTON from this patch since there's
> no testpmd change in there that requires it. There's no tolerance for dead
> code in testpmd as it doesn't expose an API.
>
> Thanks.
>
> [1] 3.14 "object"
>      http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
>
  
Dekel Peled July 2, 2019, 10:15 a.m. UTC | #5
Thanks, PSB.

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Tuesday, July 2, 2019 1:12 PM
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Dekel Peled
> <dekelp@mellanox.com>
> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; Yongseok Koh <yskoh@mellanox.com>;
> Shahaf Shuler <shahafs@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: support action with any config
> object type
> 
> On 02.07.2019 12:57, Adrien Mazarguil wrote:
> > On Tue, Jul 02, 2019 at 08:42:41AM +0000, Dekel Peled wrote:
> >> Thanks, PSB.
> >>
> >>> -----Original Message-----
> >>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>> Sent: Tuesday, July 2, 2019 11:09 AM
> >>> To: Dekel Peled <dekelp@mellanox.com>; Adrien Mazarguil
> >>> <adrien.mazarguil@6wind.com>; wenzhuo.lu@intel.com;
> >>> jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
> >>> <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>;
> Slava
> >>> Ovsiienko <viacheslavo@mellanox.com>; arybchenko@solarflare.com
> >>> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>
> >>> Subject: Re: [dpdk-dev] [PATCH] ethdev: support action with any
> >>> config object type
> >>>
> >>> On 01.07.2019 17:10, Dekel Peled wrote:
> >>>> In current implementation, an action which requires parameters must
> >>>> accept them enclosed in a structure.
> >>>> Some actions require a single, trivial type parameter, but it still
> >>>> must be enclosed in a structure.
> >>>> This obligation results in multiple, action-specific structures,
> >>>> each containing a single trivial type parameter.
> >>>>
> >>>> This patch introduces a new approach, allowing an action
> >>>> configuration object of any type, trivial or a structure.
> >>>>
> >>>> This patch introduces, in test-pmd, a new macro ARG_ENTRY_HTON, to
> >>>> allow using a single argument, not enclosed in a structure.
> >>>>
> >>>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> >>> The term "object" confuses me a bit, but I'm not a native speaker so
> >>> it could be just my wrong association. I'd prefer "configuration data".
> >> In previous version I wrote just "action configuration", and changed to
> "action configuration object" per Adrien's suggestion. I think it is better, but if
> it causes confusion maybe it should be changed.
> >>
> >> Adrien, what do you think? Does "configuration data" carry the correct
> meaning?
> > Well I'm no native speaker either but "object" is the term used in the
> > C standard with a well-defined meaning [1] and encompasses everything
> > (integers, floats, structures, unions, functions, pointers, arrays):
> >
> >   "region of data storage in the execution environment, the contents of
> which
> >    can represent values"
> >
> > I think it's a bit less vague than "data" because whenever objects are
> > mentioned in the standard, they always have a type. There's no such
> > thing as a C object without one, and rte_flow puts a lot of emphasis
> > on documenting them.
> >
> >   int foo;
> >   struct { ... } foo;
> >   double foo;
> >   char foo[];
> >   void *foo;
> >
> > Whatever the type, would you refer to "foo" itself as an "object" or
> > as "data"?
> 
> Adrien, thanks a lot. Now "object" looks OK and better than "data".

Agreed, staying with "object".

> 
> > Unrelated, but you must remove ARG_ENTRY_HTON from this patch since
> > there's no testpmd change in there that requires it. There's no
> > tolerance for dead code in testpmd as it doesn't expose an API.
> >
> > Thanks.
> >
> > [1] 3.14 "object"
> >
> >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> open-
> std.org%2Fjtc1%2Fsc22%2Fwg14%2Fwww%2Fdocs%2Fn1256.pdf&amp;data
> =02%7C01%7Cdekelp%40mellanox.com%7C65a76c486a4441d6a05e08d6fed5
> ba8d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C6369765912801
> 35692&amp;sdata=INAfwwiVUvf6c19OcTiW1K27qeIHhrk7Nk2uVu3EDjA%3D
> &amp;reserved=0
> >
  
Dekel Peled July 2, 2019, 10:17 a.m. UTC | #6
Thanks, PSB.

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Tuesday, July 2, 2019 12:57 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>;
> wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; Yongseok Koh <yskoh@mellanox.com>;
> Shahaf Shuler <shahafs@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: support action with any config
> object type
> 
> On Tue, Jul 02, 2019 at 08:42:41AM +0000, Dekel Peled wrote:
> > Thanks, PSB.
> >
> > > -----Original Message-----
> > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > Sent: Tuesday, July 2, 2019 11:09 AM
> > > To: Dekel Peled <dekelp@mellanox.com>; Adrien Mazarguil
> > > <adrien.mazarguil@6wind.com>; wenzhuo.lu@intel.com;
> > > jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
> > > <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Slava
> > > Ovsiienko <viacheslavo@mellanox.com>; arybchenko@solarflare.com
> > > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>
> > > Subject: Re: [dpdk-dev] [PATCH] ethdev: support action with any
> > > config object type
> > >
> > > On 01.07.2019 17:10, Dekel Peled wrote:
> > > > In current implementation, an action which requires parameters
> > > > must accept them enclosed in a structure.
> > > > Some actions require a single, trivial type parameter, but it
> > > > still must be enclosed in a structure.
> > > > This obligation results in multiple, action-specific structures,
> > > > each containing a single trivial type parameter.
> > > >
> > > > This patch introduces a new approach, allowing an action
> > > > configuration object of any type, trivial or a structure.
> > > >
> > > > This patch introduces, in test-pmd, a new macro ARG_ENTRY_HTON, to
> > > > allow using a single argument, not enclosed in a structure.
> > > >
> > > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > >
> > > The term "object" confuses me a bit, but I'm not a native speaker so
> > > it could be just my wrong association. I'd prefer "configuration data".
> >
> > In previous version I wrote just "action configuration", and changed to
> "action configuration object" per Adrien's suggestion. I think it is better, but if
> it causes confusion maybe it should be changed.
> >
> > Adrien, what do you think? Does "configuration data" carry the correct
> meaning?
> 
> Well I'm no native speaker either but "object" is the term used in the C
> standard with a well-defined meaning [1] and encompasses everything
> (integers, floats, structures, unions, functions, pointers, arrays):
> 
>  "region of data storage in the execution environment, the contents of which
>   can represent values"
> 
> I think it's a bit less vague than "data" because whenever objects are
> mentioned in the standard, they always have a type. There's no such thing as
> a C object without one, and rte_flow puts a lot of emphasis on documenting
> them.
> 
>  int foo;
>  struct { ... } foo;
>  double foo;
>  char foo[];
>  void *foo;
> 
> Whatever the type, would you refer to "foo" itself as an "object" or as
> "data"?

Understood, staying with "object".

> 
> Unrelated, but you must remove ARG_ENTRY_HTON from this patch since
> there's no testpmd change in there that requires it. There's no tolerance for
> dead code in testpmd as it doesn't expose an API.

OK, I'll remove it from this patch and add it to the relevant series.

> 
> Thanks.
> 
> [1] 3.14 "object"
> 
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> open-
> std.org%2Fjtc1%2Fsc22%2Fwg14%2Fwww%2Fdocs%2Fn1256.pdf&amp;data
> =02%7C01%7Cdekelp%40mellanox.com%7C305d2ae37a3b411ea91608d6fed3
> afbf%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C63697658247762
> 5253&amp;sdata=8wcANpiGcjbhVMFHWs1wRB%2FnkvoSOawmHm8bkavY2
> U4%3D&amp;reserved=0
> 
> --
> Adrien Mazarguil
> 6WIND
  

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 201bd9d..fa7bd13 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -485,6 +485,14 @@  struct token {
 		.size = sizeof(((s *)0)->f), \
 	})
 
+/** Same as ARGS_ENTRY_HTON() for a single argument, without structure. */
+#define ARG_ENTRY_HTON(s) \
+	(&(const struct arg){ \
+		.hton = 1, \
+		.offset = 0, \
+		.size = sizeof(s), \
+	})
+
 /** Parser output buffer layout expected by cmd_flow_parsed(). */
 struct buffer {
 	enum index command; /**< Flow command. */
diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index a34d012..67deed7 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1213,8 +1213,9 @@  Matches an application specific 32 bit metadata item.
 Actions
 ~~~~~~~
 
-Each possible action is represented by a type. Some have associated
-configuration structures. Several actions combined in a list can be assigned
+Each possible action is represented by a type.
+An action can have an associated configuration object.
+Several actions combined in a list can be assigned
 to a flow rule and are performed in order.
 
 They fall in three categories:
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index f3a8fb1..2232856 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -1244,9 +1244,10 @@  struct rte_flow_item {
 /**
  * Action types.
  *
- * Each possible action is represented by a type. Some have associated
- * configuration structures. Several actions combined in a list can be
- * assigned to a flow rule and are performed in order.
+ * Each possible action is represented by a type.
+ * An action can have an associated configuration object.
+ * Several actions combined in a list can be assigned
+ * to a flow rule and are performed in order.
  *
  * They fall in three categories:
  *
@@ -2136,11 +2137,11 @@  struct rte_flow_action_set_mac {
  *
  * A list of actions is terminated by a END action.
  *
- * For simple actions without a configuration structure, conf remains NULL.
+ * For simple actions without a configuration object, conf remains NULL.
  */
 struct rte_flow_action {
 	enum rte_flow_action_type type; /**< Action type. */
-	const void *conf; /**< Pointer to action configuration structure. */
+	const void *conf; /**< Pointer to action configuration object. */
 };
 
 /**