[ovs-dev] [PATCH 1/2] netdev-dpdk: Drop offload API for vhost ports.

Ophir Munk ophirmu at mellanox.com
Wed Oct 31 17:01:20 UTC 2018


Hi,
It is a good idea to remove the macro from the h file.
Please find more comments inline

> -----Original Message-----
> From: Lam, Tiago [mailto:tiago.lam at intel.com]
> Sent: Wednesday, October 31, 2018 2:41 PM
> To: Stokes, Ian <ian.stokes at intel.com>; Ilya Maximets
> <i.maximets at samsung.com>; ovs-dev at openvswitch.org; Ophir Munk
> <ophirmu at mellanox.com>
> Cc: Finn Christensen <fc at napatech.com>; Yuanhan Liu
> <yliu at fridaylinux.org>; Shahaf Shuler <shahafs at mellanox.com>; Chandran,
> Sugesh <sugesh.chandran at intel.com>
> Subject: Re: [PATCH 1/2] netdev-dpdk: Drop offload API for vhost ports.
> 
> On 31/10/2018 11:40, Stokes, Ian wrote:
> >> On 31.10.2018 13:42, Stokes, Ian wrote:
> >>>> vhost ports are not DPDK eth ports and has no rte_flow API.
> >>>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time
> >>>> wasting and errors in log.
> >>>>
> >>>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file,
> >>>> because there is no need to expose it in header.
> >>>
> >>> Adding Ophir and Tiago as they are both looking at the HWOL work at
> >>> the
> >> moment.
> >>>
> >>>>
> >>>> CC: Finn Christensen <fc at napatech.com>
> >>>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte
> >>>> flow")
> >>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> >>>> ---
> >>>>  lib/netdev-dpdk.c | 10 +++++++---
> >>>>  lib/netdev-dpdk.h |  4 ----
> >>>>  2 files changed, 7 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >>>> f91aa27cd..7fe5eb087 100644
> >>>> --- a/lib/netdev-dpdk.c
> >>>> +++ b/lib/netdev-dpdk.c
> >>>> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev
> *netdev,
> >>>> const
> >>>> ovs_u128 *ufid,
> >>>>                                          ufid, rte_flow);  }
> >>>>
> >>>> +#define DPDK_FLOW_OFFLOAD_API                   \
> >>>> +    .flow_put = netdev_dpdk_flow_put,           \
> >>>> +    .flow_del = netdev_dpdk_flow_del
> >>>> +

Regarding the macro name - I suggest using DPDK_FLOW_API.
The offload promise is to scale with the HW abilities. 
If the HW cannot support the flow there would be no offload (just normal OVS operation).
So better omitting "OFFLOAD" from the macro name.

> >>>>  #define NETDEV_DPDK_CLASS_COMMON                            \
> >>>>      .is_pmd = true,                                         \
> >>>>      .alloc = netdev_dpdk_alloc,                             \
> >>>> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev
> *netdev,
> >>>> const
> >>>> ovs_u128 *ufid,
> >>>>      .rxq_alloc = netdev_dpdk_rxq_alloc,                     \
> >>>>      .rxq_construct = netdev_dpdk_rxq_construct,             \
> >>>>      .rxq_destruct = netdev_dpdk_rxq_destruct,               \
> >>>> -    .rxq_dealloc = netdev_dpdk_rxq_dealloc,                 \
> >>>> -    DPDK_FLOW_OFFLOAD_API
> >>>> +    .rxq_dealloc = netdev_dpdk_rxq_dealloc
> >>>>
> >>>>  #define NETDEV_DPDK_CLASS_BASE                          \
> >>>>      NETDEV_DPDK_CLASS_COMMON,                           \
> >>>> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev
> *netdev,
> >>>> const
> >>>> ovs_u128 *ufid,
> >>>>      .get_features = netdev_dpdk_get_features,           \
> >>>>      .get_status = netdev_dpdk_get_status,               \
> >>>>      .reconfigure = netdev_dpdk_reconfigure,             \
> >>>> -    .rxq_recv = netdev_dpdk_rxq_recv
> >>>> +    .rxq_recv = netdev_dpdk_rxq_recv,                   \
> >>>> +    DPDK_FLOW_OFFLOAD_API
> >>>>

Here is the only place where the new macro is used and it only has 2 callback assignments.
What do you say about directly assigning the 2 callbacks and giving up the macro?
Something like:
-    .rxq_recv = netdev_dpdk_rxq_recv
+    .rxq_recv = netdev_dpdk_rxq_recv,                   \
-    DPDK_FLOW_OFFLOAD_API
+    .flow_put = netdev_dpdk_flow_put,           \
+    .flow_del = netdev_dpdk_flow_del

> >>>>  static const struct netdev_class dpdk_class = {
> >>>>      .type = "dpdk",
> >>>> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index
> >>>> cc0501d68..b7d02a77d 100644
> >>>> --- a/lib/netdev-dpdk.h
> >>>> +++ b/lib/netdev-dpdk.h
> >>>> @@ -25,10 +25,6 @@ struct dp_packet;
> >>>>
> >>>>  #ifdef DPDK_NETDEV
> >>>>
> >>>> -#define DPDK_FLOW_OFFLOAD_API                   \
> >>>> -    .flow_put = netdev_dpdk_flow_put,           \
> >>>> -    .flow_del = netdev_dpdk_flow_del
> >>>> -
> >>>
> >>> I believe the original reason for placing them here was to keep the
> >> approach uniform across the netdevs WRT where the APIs are defined.
> >>>
> >>> I believe the expectation is that they will be shared outside of
> >>> netdev-
> >> dpdk at some point. That work in ongoing.
> >>>
> >>>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fm
> >>> ail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2018-
> August%2F350510.htm
> >>>
> l&amp;data=02%7C01%7Cophirmu%40mellanox.com%7Cbc9e78988b7b4bf8
> b82408
> >>>
> d63f2e28d6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63676
> 5864828
> >>>
> 187556&amp;sdata=LiGeqC84qM6VCFUtRgKpLv%2BBrq8if4Kyto8mw2EaNQ
> s%3D&am
> >>> p;reserved=0
> >>>
> >>> We can remove it from mast but FYI it will be back I suspect.
> >>
> >> The functions are static to netdev-dpdk.c. If you're going to create
> >> new DPDK netdev in a separate file, you will have to move much more
> >> code and expose many functions in a header file.
> >> netdev-linux has the definition in header, because functions are
> >> exposed in header too. Here we have just strange define which has no
> >> sense for any file that includes netdev-dpdk.h and could not be used
> >> anyway without exposing these static functions in header.
> >>
> >> IMHO, with this change applied, future full offloading patches will
> >> be more consistent.
> >
> > OK, that makes sense with what we have to date so I won't block the
> patch. In previous discussions WRT HWOL there was talk that it would be
> moved  to its own HWOL module, but we can cross that bridge when we
> come to it.
> 
> I agree. From what I understand the partial offload implementation will have
> to be integrated with the full offload solution that has been under
> development, and which will have its own module. Thus, at that point, it is
> likely that these definitions can be part of the new module for hardware
> offload.
> 

With representors ports (since dpdk 18.08) it seems we can have the current class dpdk for HW offload.
The representors ports hide the HW details and make them appear as the same  dpdk ports to OVS.
However, this is a different discussion ...

> Thanks,
> Tiago.
> 
> >
> > Ian
> >>
> >>>
> >>> Ian
> >>>
> >>>>  void netdev_dpdk_register(void);
> >>>>  void free_dpdk_buf(struct dp_packet *);
> >>>>
> >>>> --
> >>>> 2.17.1
> >>>
> >>>
> >>>


More information about the dev mailing list