> On 31.10.2018 20:01, Ophir Munk wrote: > > 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:[email protected]] > >> Sent: Wednesday, October 31, 2018 2:41 PM > >> To: Stokes, Ian <[email protected]>; Ilya Maximets > >> <[email protected]>; [email protected]; Ophir Munk > >> <[email protected]> > >> Cc: Finn Christensen <[email protected]>; Yuanhan Liu > >> <[email protected]>; Shahaf Shuler <[email protected]>; > >> Chandran, Sugesh <[email protected]> > >> 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 <[email protected]> > >>>>>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with > >>>>>> rte > >>>>>> flow") > >>>>>> Signed-off-by: Ilya Maximets <[email protected]> > >>>>>> --- > >>>>>> 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. > > IMHO, it's better not to rename. At first, all the corresponding macro > definitions for other netdevs has OFFLOAD in their names. At second, this > API is intended for offloading and has no sense without it. > Most of the users of this API has 'offload' in function and data structure > names.
+1, let's keep them uniform across the netdevs. Ian > > > > >>>>>> #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 > > Not sure. Having a macro here groups the offload API together and makes it > visible for a reader. And, probably, we'll implement more offload APIs in > the future and there will be more functions. > I'd agree, I believe in the original patch to simplify the netdev classes proposed something similar to using callbacks but ultimately it was decided that the API would be expanded in the future so should be kept. Ian _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
