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

[Lam, Tiago]

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
>>>> +
>>>>  #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
>>>>
>>>>  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://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350510.html
>>>
>>> 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.

Thanks,
Tiago.

> 
> Ian
>>
>>>
>>> Ian
>>>
>>>>  void netdev_dpdk_register(void);
>>>>  void free_dpdk_buf(struct dp_packet *);
>>>>
>>>> --
>>>> 2.17.1
>>>
>>>
>>>