Re: [ovs-dev] [PATCH 5/5] netdev: Clean up class initialization.

2018-08-15 Thread Ben Pfaff 
On Wed, Aug 01, 2018 at 03:23:38PM +0100, Ian Stokes wrote:
> On 7/12/2018 10:55 PM, Ben Pfaff wrote:
> >The macros are hard to read.  This makes it a little more readable.
> >
> 
> Thanks for this Ben, one minor comment below.
> 
> >Signed-off-by: Ben Pfaff 
> >---
> >  configure.ac  |   1 +
> >  lib/netdev-dpdk.c | 235 --
> >  lib/netdev-dummy.c| 134 
> >  lib/netdev-linux.c| 340 
> > +++---
> >  lib/netdev-linux.h|  18 +--
> >  lib/netdev-provider.h |   2 -
> >  lib/netdev-vport.c| 223 +++--
> >  7 files changed, 369 insertions(+), 584 deletions(-)
> >
> >diff --git a/configure.ac b/configure.ac
> >index c89c607c7124..66281c4d6811 100644
> >--- a/configure.ac
> >+++ b/configure.ac
> >@@ -172,6 +172,7 @@ OVS_ENABLE_OPTION([-Wduplicated-cond])
> >  OVS_ENABLE_OPTION([-Qunused-arguments])
> >  OVS_ENABLE_OPTION([-Wshadow])
> >  OVS_ENABLE_OPTION([-Wno-null-pointer-arithmetic])
> >+OVS_ENABLE_OPTION([-Warray-bounds-pointer-arithmetic])
> >  OVS_CONDITIONAL_CC_OPTION([-Wno-unused], [HAVE_WNO_UNUSED])
> >  OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter], 
> > [HAVE_WNO_UNUSED_PARAMETER])
> >  OVS_ENABLE_WERROR
> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >index 9bf21856075b..4de4cf116e92 100644
> >--- a/lib/netdev-dpdk.c
> >+++ b/lib/netdev-dpdk.c
> >@@ -4695,161 +4695,86 @@ netdev_dpdk_flow_del(struct netdev *netdev, const 
> >ovs_u128 *ufid,
> >  ufid, rte_flow);
> >  }
> >-#define DPDK_FLOW_OFFLOAD_API \
> >-NULL,   /* flow_flush */  \
> >-NULL,   /* flow_dump_create */\
> >-NULL,   /* flow_dump_destroy */   \
> >-NULL,   /* flow_dump_next */  \
> >-netdev_dpdk_flow_put, \
> >-NULL,   /* flow_get */\
> >-netdev_dpdk_flow_del, \
> >-NULL/* init_flow_api */
> 
> Not sure if DPDK_FLOW_OFFLOAD_API should be completely removed, as I
> understand it the remaining offload functionality is currently being worked
> on with a view to enable full HW offload so they will be re-introduced in
> the future.
> 
> The macro could be moved from here to netdev-dpdk.h and then added to the
> NETDEV_DPDK_CLASS_BASE macro you introduce below (this would be similar to
> what is implemented for netdev-linux, and a more uniform approach across the
> netdevs).
> 
> Sugesh, you've been involved in the HW full offload work, do you have an
> opinion on this?

OK.  I folded in the following and reposted v2 as:
https://patchwork.ozlabs.org/patch/957990/

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 917b3b82c88e..628d75a75024 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4715,8 +4715,7 @@ netdev_dpdk_flow_del(struct netdev *netdev, const 
ovs_u128 *ufid,
 .rxq_construct = netdev_dpdk_rxq_construct, \
 .rxq_destruct = netdev_dpdk_rxq_destruct,   \
 .rxq_dealloc = netdev_dpdk_rxq_dealloc, \
-.flow_put = netdev_dpdk_flow_put,   \
-.flow_del = netdev_dpdk_flow_del
+DPDK_FLOW_OFFLOAD_API
 
 #define NETDEV_DPDK_CLASS_BASE  \
 NETDEV_DPDK_CLASS_COMMON,   \
diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
index b7d02a77dc72..cc0501d6879a 100644
--- a/lib/netdev-dpdk.h
+++ b/lib/netdev-dpdk.h
@@ -25,6 +25,10 @@ struct dp_packet;
 
 #ifdef DPDK_NETDEV
 
+#define DPDK_FLOW_OFFLOAD_API   \
+.flow_put = netdev_dpdk_flow_put,   \
+.flow_del = netdev_dpdk_flow_del
+
 void netdev_dpdk_register(void);
 void free_dpdk_buf(struct dp_packet *);
 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 5/5] netdev: Clean up class initialization.

2018-08-02 Thread Chandran, Sugesh 
Hi Ian/Ben ,

Please find my comments below.

Regards
_Sugesh

> -Original Message-
> From: Stokes, Ian
> Sent: Wednesday, August 1, 2018 3:24 PM
> To: Ben Pfaff ; d...@openvswitch.org
> Cc: Chandran, Sugesh 
> Subject: Re: [ovs-dev] [PATCH 5/5] netdev: Clean up class initialization.
> 
> On 7/12/2018 10:55 PM, Ben Pfaff wrote:
> > The macros are hard to read.  This makes it a little more readable.
> >
> 
> Thanks for this Ben, one minor comment below.
> 
> > Signed-off-by: Ben Pfaff 
> > ---
> >
> > -#define DPDK_FLOW_OFFLOAD_API \
> > -NULL,   /* flow_flush */  \
> > -NULL,   /* flow_dump_create */\
> > -NULL,   /* flow_dump_destroy */   \
> > -NULL,   /* flow_dump_next */  \
> > -netdev_dpdk_flow_put, \
> > -NULL,   /* flow_get */\
> > -netdev_dpdk_flow_del, \
> > -NULL/* init_flow_api */
> 
> Not sure if DPDK_FLOW_OFFLOAD_API should be completely removed, as I
> understand it the remaining offload functionality is currently being
> worked on with a view to enable full HW offload so they will be
> re-introduced in the future.
> 
> The macro could be moved from here to netdev-dpdk.h and then added to
> the NETDEV_DPDK_CLASS_BASE macro you introduce below (this would be
> similar to what is implemented for netdev-linux, and a more uniform
> approach across the netdevs).
> 
> Sugesh, you've been involved in the HW full offload work, do you have an
> opinion on this?
> 
> Ian
[Sugesh] I agree with Ian here, We are looking at full offload solutions in DPDK
Datapath which need to use the remaining flow related APIs for the flow 
management in the
hardware. So I think it make sense to keep it uniform across different netdevs.

> > -
> > -
> > -#define NETDEV_DPDK_CLASS(NAME, INIT, CONSTRUCT, DESTRUCT,\
> > -  SET_CONFIG, SET_TX_MULTIQ, SEND,\
> > -  GET_CARRIER, GET_STATS,\
> > -  GET_CUSTOM_STATS,
> >   \
> > -  GET_FEATURES, GET_STATUS,   \
> > -  RECONFIGURE, RXQ_RECV)  \
> > -{ \

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 5/5] netdev: Clean up class initialization.

2018-08-01 Thread Ian Stokes 

On 7/12/2018 10:55 PM, Ben Pfaff wrote:

The macros are hard to read.  This makes it a little more readable.



Thanks for this Ben, one minor comment below.


Signed-off-by: Ben Pfaff 
---
  configure.ac  |   1 +
  lib/netdev-dpdk.c | 235 --
  lib/netdev-dummy.c| 134 
  lib/netdev-linux.c| 340 +++---
  lib/netdev-linux.h|  18 +--
  lib/netdev-provider.h |   2 -
  lib/netdev-vport.c| 223 +++--
  7 files changed, 369 insertions(+), 584 deletions(-)

diff --git a/configure.ac b/configure.ac
index c89c607c7124..66281c4d6811 100644
--- a/configure.ac
+++ b/configure.ac
@@ -172,6 +172,7 @@ OVS_ENABLE_OPTION([-Wduplicated-cond])
  OVS_ENABLE_OPTION([-Qunused-arguments])
  OVS_ENABLE_OPTION([-Wshadow])
  OVS_ENABLE_OPTION([-Wno-null-pointer-arithmetic])
+OVS_ENABLE_OPTION([-Warray-bounds-pointer-arithmetic])
  OVS_CONDITIONAL_CC_OPTION([-Wno-unused], [HAVE_WNO_UNUSED])
  OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter], 
[HAVE_WNO_UNUSED_PARAMETER])
  OVS_ENABLE_WERROR
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 9bf21856075b..4de4cf116e92 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4695,161 +4695,86 @@ netdev_dpdk_flow_del(struct netdev *netdev, const 
ovs_u128 *ufid,
  ufid, rte_flow);
  }
  
-#define DPDK_FLOW_OFFLOAD_API \

-NULL,   /* flow_flush */  \
-NULL,   /* flow_dump_create */\
-NULL,   /* flow_dump_destroy */   \
-NULL,   /* flow_dump_next */  \
-netdev_dpdk_flow_put, \
-NULL,   /* flow_get */\
-netdev_dpdk_flow_del, \
-NULL/* init_flow_api */


Not sure if DPDK_FLOW_OFFLOAD_API should be completely removed, as I 
understand it the remaining offload functionality is currently being 
worked on with a view to enable full HW offload so they will be 
re-introduced in the future.


The macro could be moved from here to netdev-dpdk.h and then added to 
the NETDEV_DPDK_CLASS_BASE macro you introduce below (this would be 
similar to what is implemented for netdev-linux, and a more uniform 
approach across the netdevs).


Sugesh, you've been involved in the HW full offload work, do you have an 
opinion on this?


Ian

-
-
-#define NETDEV_DPDK_CLASS(NAME, INIT, CONSTRUCT, DESTRUCT,\
-  SET_CONFIG, SET_TX_MULTIQ, SEND,\
-  GET_CARRIER, GET_STATS,\
-  GET_CUSTOM_STATS,
  \
-  GET_FEATURES, GET_STATUS,   \
-  RECONFIGURE, RXQ_RECV)  \
-{ \
-NAME, \
-true,   /* is_pmd */  \
-INIT,   /* init */\
-NULL,   /* netdev_dpdk_run */ \
-NULL,   /* netdev_dpdk_wait */\
-  \
-netdev_dpdk_alloc,\
-CONSTRUCT,\
-DESTRUCT, \
-netdev_dpdk_dealloc,  \
-netdev_dpdk_get_config,   \
-SET_CONFIG,   \
-NULL,   /* get_tunnel_config */   \
-NULL,   /* build header */\
-NULL,   /* push header */ \
-NULL,   /* pop header */  \
-netdev_dpdk_get_numa_id,/* get_numa_id */ \
-SET_TX_MULTIQ,\
-  \
-SEND,   /* send */\
-NULL,   /* send_wait */   \
-  \
-netdev_dpdk_set_etheraddr,\
-netdev_dpdk_get_etheraddr,\
-netdev_dpdk_get_mtu,  \
-netdev_dpdk_set_mtu,  \
-netdev_dpdk_get_ifindex,  \
-GET_CARRIER,  \
-netdev_dpdk_get_carrier_resets,   \
-netdev_dpdk_set_miimon,

Re: [ovs-dev] [PATCH 5/5] netdev: Clean up class initialization.

2018-07-16 Thread Yifeng Sun 
Thanks. Looks good to me.

Reviewed-by: Yifeng Sun 

On Thu, Jul 12, 2018 at 2:55 PM, Ben Pfaff  wrote:

> The macros are hard to read.  This makes it a little more readable.
>
> Signed-off-by: Ben Pfaff 
> ---
>  configure.ac  |   1 +
>  lib/netdev-dpdk.c | 235 --
>  lib/netdev-dummy.c| 134 
>  lib/netdev-linux.c| 340 +++---
> 
>  lib/netdev-linux.h|  18 +--
>  lib/netdev-provider.h |   2 -
>  lib/netdev-vport.c| 223 +++--
>  7 files changed, 369 insertions(+), 584 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index c89c607c7124..66281c4d6811 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -172,6 +172,7 @@ OVS_ENABLE_OPTION([-Wduplicated-cond])
>  OVS_ENABLE_OPTION([-Qunused-arguments])
>  OVS_ENABLE_OPTION([-Wshadow])
>  OVS_ENABLE_OPTION([-Wno-null-pointer-arithmetic])
> +OVS_ENABLE_OPTION([-Warray-bounds-pointer-arithmetic])
>  OVS_CONDITIONAL_CC_OPTION([-Wno-unused], [HAVE_WNO_UNUSED])
>  OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter],
> [HAVE_WNO_UNUSED_PARAMETER])
>  OVS_ENABLE_WERROR
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 9bf21856075b..4de4cf116e92 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4695,161 +4695,86 @@ netdev_dpdk_flow_del(struct netdev *netdev, const
> ovs_u128 *ufid,
>  ufid, rte_flow);
>  }
>
> -#define DPDK_FLOW_OFFLOAD_API \
> -NULL,   /* flow_flush */  \
> -NULL,   /* flow_dump_create */\
> -NULL,   /* flow_dump_destroy */   \
> -NULL,   /* flow_dump_next */  \
> -netdev_dpdk_flow_put, \
> -NULL,   /* flow_get */\
> -netdev_dpdk_flow_del, \
> -NULL/* init_flow_api */
> -
> -
> -#define NETDEV_DPDK_CLASS(NAME, INIT, CONSTRUCT, DESTRUCT,\
> -  SET_CONFIG, SET_TX_MULTIQ, SEND,\
> -  GET_CARRIER, GET_STATS,\
> -  GET_CUSTOM_STATS,
> \
> -  GET_FEATURES, GET_STATUS,   \
> -  RECONFIGURE, RXQ_RECV)  \
> -{ \
> -NAME, \
> -true,   /* is_pmd */  \
> -INIT,   /* init */\
> -NULL,   /* netdev_dpdk_run */ \
> -NULL,   /* netdev_dpdk_wait */\
> -  \
> -netdev_dpdk_alloc,\
> -CONSTRUCT,\
> -DESTRUCT, \
> -netdev_dpdk_dealloc,  \
> -netdev_dpdk_get_config,   \
> -SET_CONFIG,   \
> -NULL,   /* get_tunnel_config */   \
> -NULL,   /* build header */\
> -NULL,   /* push header */ \
> -NULL,   /* pop header */  \
> -netdev_dpdk_get_numa_id,/* get_numa_id */ \
> -SET_TX_MULTIQ,\
> -  \
> -SEND,   /* send */\
> -NULL,   /* send_wait */   \
> -  \
> -netdev_dpdk_set_etheraddr,\
> -netdev_dpdk_get_etheraddr,\
> -netdev_dpdk_get_mtu,  \
> -netdev_dpdk_set_mtu,  \
> -netdev_dpdk_get_ifindex,  \
> -GET_CARRIER,  \
> -netdev_dpdk_get_carrier_resets,   \
> -netdev_dpdk_set_miimon,   \
> -GET_STATS,\
> -GET_CUSTOM_STATS,
> \
> -GET_FEATURES, \
> -NULL,   /* set_advertisements */  \
> -NULL,   /* get_pt_mode */ \
> -  \
> -netdev_dpdk_set_policing,

[ovs-dev] [PATCH 5/5] netdev: Clean up class initialization.

2018-07-12 Thread Ben Pfaff 
The macros are hard to read.  This makes it a little more readable.

Signed-off-by: Ben Pfaff 
---
 configure.ac  |   1 +
 lib/netdev-dpdk.c | 235 --
 lib/netdev-dummy.c| 134 
 lib/netdev-linux.c| 340 +++---
 lib/netdev-linux.h|  18 +--
 lib/netdev-provider.h |   2 -
 lib/netdev-vport.c| 223 +++--
 7 files changed, 369 insertions(+), 584 deletions(-)

diff --git a/configure.ac b/configure.ac
index c89c607c7124..66281c4d6811 100644
--- a/configure.ac
+++ b/configure.ac
@@ -172,6 +172,7 @@ OVS_ENABLE_OPTION([-Wduplicated-cond])
 OVS_ENABLE_OPTION([-Qunused-arguments])
 OVS_ENABLE_OPTION([-Wshadow])
 OVS_ENABLE_OPTION([-Wno-null-pointer-arithmetic])
+OVS_ENABLE_OPTION([-Warray-bounds-pointer-arithmetic])
 OVS_CONDITIONAL_CC_OPTION([-Wno-unused], [HAVE_WNO_UNUSED])
 OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter], [HAVE_WNO_UNUSED_PARAMETER])
 OVS_ENABLE_WERROR
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 9bf21856075b..4de4cf116e92 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4695,161 +4695,86 @@ netdev_dpdk_flow_del(struct netdev *netdev, const 
ovs_u128 *ufid,
 ufid, rte_flow);
 }
 
-#define DPDK_FLOW_OFFLOAD_API \
-NULL,   /* flow_flush */  \
-NULL,   /* flow_dump_create */\
-NULL,   /* flow_dump_destroy */   \
-NULL,   /* flow_dump_next */  \
-netdev_dpdk_flow_put, \
-NULL,   /* flow_get */\
-netdev_dpdk_flow_del, \
-NULL/* init_flow_api */
-
-
-#define NETDEV_DPDK_CLASS(NAME, INIT, CONSTRUCT, DESTRUCT,\
-  SET_CONFIG, SET_TX_MULTIQ, SEND,\
-  GET_CARRIER, GET_STATS,\
-  GET_CUSTOM_STATS,
  \
-  GET_FEATURES, GET_STATUS,   \
-  RECONFIGURE, RXQ_RECV)  \
-{ \
-NAME, \
-true,   /* is_pmd */  \
-INIT,   /* init */\
-NULL,   /* netdev_dpdk_run */ \
-NULL,   /* netdev_dpdk_wait */\
-  \
-netdev_dpdk_alloc,\
-CONSTRUCT,\
-DESTRUCT, \
-netdev_dpdk_dealloc,  \
-netdev_dpdk_get_config,   \
-SET_CONFIG,   \
-NULL,   /* get_tunnel_config */   \
-NULL,   /* build header */\
-NULL,   /* push header */ \
-NULL,   /* pop header */  \
-netdev_dpdk_get_numa_id,/* get_numa_id */ \
-SET_TX_MULTIQ,\
-  \
-SEND,   /* send */\
-NULL,   /* send_wait */   \
-  \
-netdev_dpdk_set_etheraddr,\
-netdev_dpdk_get_etheraddr,\
-netdev_dpdk_get_mtu,  \
-netdev_dpdk_set_mtu,  \
-netdev_dpdk_get_ifindex,  \
-GET_CARRIER,  \
-netdev_dpdk_get_carrier_resets,   \
-netdev_dpdk_set_miimon,   \
-GET_STATS,\
-GET_CUSTOM_STATS,  
  \
-GET_FEATURES, \
-NULL,   /* set_advertisements */  \
-NULL,   /* get_pt_mode */ \
-  \
-netdev_dpdk_set_policing, \
-netdev_dpdk_get_qos_types,\
-NULL,   /* get_qos_capabilities */\
-netdev_dpdk_get_qos,