diff mbox

[ovs-dev,ovs,V4,00/24] Introducing HW offload support for openvswitch

Message ID 20170316154038.GA11529@penelope.horms.nl
State Not Applicable
Headers show

Commit Message

Simon Horman March 16, 2017, 3:40 p.m. UTC 
On Mon, Mar 13, 2017 at 03:36:50PM +0200, Roi Dayan wrote:
> This patch series introduces rule offload functionality to dpif-netlink
> via netdev ports new flow offloading API. The user can specify whether to
> enable rule offloading or not via OVS configuration. Netdev providers
> are able to implement netdev flow offload API in order to offload rules.
> 
> This patch series also implements one offload scheme for netdev-linux,
> using TC flower classifier, which was chosen because its sort of natural
> to state OVS DP rules for this classifier. However, the code can be
> extended to support other classifiers such as U32, eBPF, etc which support
> offload as well.
> 
> The use-case we are currently addressing is the newly sriov switchdev mode
> in the Linux kernel which was introduced in version 4.8 [1][2].
> This series was tested against sriov vfs vports representors of the
> Mellanox 100G ConnectX-4 series exposed by the mlx5 kernel driver.
> 
> 
> V3->V4:
>     - Move declarations to the right commit with implementation
>     - Fix tc_get_flower flow return false success 
>     - Fix memory leaks - not releasing tc_transact replies
>     - Fix travis failure for OSX compilation
>     - Fix loop in dpif_netlink_flow_dump_next
>     - Fix declared default value for tc-policy in vswitch.xml
>     - Refactor loop in netdev_tc_flow_dump_next
>     - Add missing error checks in parse_flow_put
>     - Fix handling modify request where old rule is in hw and new
>       rule is not supported and needs to be in sw.
>     - Use 2 hashmaps instead of 1 for faster reverse lookup of ufid from tc
>     - Init ports when enabling hw-offload after OVS is running
> 
>     TODO: Fix breaking of datapath compilation
>           Fix testsuite failures
> 
>     Travis
>         https://travis-ci.org/roidayan/ovs/builds/210549325
>     AppVeyor
>         https://ci.appveyor.com/project/roidayan/ovs/build/1.0.15

Hi Roi,

we have found that in our testing it seems to resolve problems flagged by
Travis. I think that if this is the way forwards then the code could
be collapsed back into netdev_vport_get_ifindex() and the surrounding
#ifdef __linux__ logic could be removed.

Comments

Roi Dayan March 20, 2017, 8:33 a.m. UTC | #1 
On 16/03/2017 17:40, Simon Horman wrote:
> On Mon, Mar 13, 2017 at 03:36:50PM +0200, Roi Dayan wrote:
>> This patch series introduces rule offload functionality to dpif-netlink
>> via netdev ports new flow offloading API. The user can specify whether to
>> enable rule offloading or not via OVS configuration. Netdev providers
>> are able to implement netdev flow offload API in order to offload rules.
>>
>> This patch series also implements one offload scheme for netdev-linux,
>> using TC flower classifier, which was chosen because its sort of natural
>> to state OVS DP rules for this classifier. However, the code can be
>> extended to support other classifiers such as U32, eBPF, etc which support
>> offload as well.
>>
>> The use-case we are currently addressing is the newly sriov switchdev mode
>> in the Linux kernel which was introduced in version 4.8 [1][2].
>> This series was tested against sriov vfs vports representors of the
>> Mellanox 100G ConnectX-4 series exposed by the mlx5 kernel driver.
>>
>>
>> V3->V4:
>>     - Move declarations to the right commit with implementation
>>     - Fix tc_get_flower flow return false success
>>     - Fix memory leaks - not releasing tc_transact replies
>>     - Fix travis failure for OSX compilation
>>     - Fix loop in dpif_netlink_flow_dump_next
>>     - Fix declared default value for tc-policy in vswitch.xml
>>     - Refactor loop in netdev_tc_flow_dump_next
>>     - Add missing error checks in parse_flow_put
>>     - Fix handling modify request where old rule is in hw and new
>>       rule is not supported and needs to be in sw.
>>     - Use 2 hashmaps instead of 1 for faster reverse lookup of ufid from tc
>>     - Init ports when enabling hw-offload after OVS is running
>>
>>     TODO: Fix breaking of datapath compilation
>>           Fix testsuite failures
>>
>>     Travis
>>         https://travis-ci.org/roidayan/ovs/builds/210549325
>>     AppVeyor
>>         https://ci.appveyor.com/project/roidayan/ovs/build/1.0.15
>
> Hi Roi,
>
> we have found that in our testing it seems to resolve problems flagged by
> Travis. I think that if this is the way forwards then the code could
> be collapsed back into netdev_vport_get_ifindex() and the surrounding
> #ifdef __linux__ logic could be removed.
>

thanks Simon! looks good.
We'll send V5 with your patches and some other fixes we did.


> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 4cc086e..d055d53 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -783,7 +783,7 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>
>  #ifdef __linux__
>  static int
> -netdev_vport_get_ifindex(const struct netdev *netdev_)
> +netdev_vport_get_ifindex__(const struct netdev *netdev_)
>  {
>      char buf[NETDEV_VPORT_NAME_BUFSIZE];
>      const char *name = netdev_vport_get_dpif_port(netdev_, buf, sizeof(buf));
> @@ -791,6 +791,15 @@ netdev_vport_get_ifindex(const struct netdev *netdev_)
>      return linux_get_ifindex(name);
>  }
>
> +static int
> +netdev_vport_get_ifindex(const struct netdev *netdev_)
> +{
> +    if (netdev_flow_api_enabled)
> +        return netdev_vport_get_ifindex__(netdev_);
> +    else
> +        return -EOPNOTSUPP;
> +}
> +
>  #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
>  #define NETDEV_FLOW_OFFLOAD_API LINUX_FLOW_OFFLOAD_API
>  #else /* !__linux__ */
>
Roi Dayan March 20, 2017, 8:56 a.m. UTC | #2 
On 20/03/2017 10:33, Roi Dayan wrote:
>
>
> On 16/03/2017 17:40, Simon Horman wrote:
>> On Mon, Mar 13, 2017 at 03:36:50PM +0200, Roi Dayan wrote:
>>> This patch series introduces rule offload functionality to dpif-netlink
>>> via netdev ports new flow offloading API. The user can specify
>>> whether to
>>> enable rule offloading or not via OVS configuration. Netdev providers
>>> are able to implement netdev flow offload API in order to offload rules.
>>>
>>> This patch series also implements one offload scheme for netdev-linux,
>>> using TC flower classifier, which was chosen because its sort of natural
>>> to state OVS DP rules for this classifier. However, the code can be
>>> extended to support other classifiers such as U32, eBPF, etc which
>>> support
>>> offload as well.
>>>
>>> The use-case we are currently addressing is the newly sriov switchdev
>>> mode
>>> in the Linux kernel which was introduced in version 4.8 [1][2].
>>> This series was tested against sriov vfs vports representors of the
>>> Mellanox 100G ConnectX-4 series exposed by the mlx5 kernel driver.
>>>
>>>
>>> V3->V4:
>>>     - Move declarations to the right commit with implementation
>>>     - Fix tc_get_flower flow return false success
>>>     - Fix memory leaks - not releasing tc_transact replies
>>>     - Fix travis failure for OSX compilation
>>>     - Fix loop in dpif_netlink_flow_dump_next
>>>     - Fix declared default value for tc-policy in vswitch.xml
>>>     - Refactor loop in netdev_tc_flow_dump_next
>>>     - Add missing error checks in parse_flow_put
>>>     - Fix handling modify request where old rule is in hw and new
>>>       rule is not supported and needs to be in sw.
>>>     - Use 2 hashmaps instead of 1 for faster reverse lookup of ufid
>>> from tc
>>>     - Init ports when enabling hw-offload after OVS is running
>>>
>>>     TODO: Fix breaking of datapath compilation
>>>           Fix testsuite failures
>>>
>>>     Travis
>>>         https://travis-ci.org/roidayan/ovs/builds/210549325
>>>     AppVeyor
>>>         https://ci.appveyor.com/project/roidayan/ovs/build/1.0.15
>>
>> Hi Roi,
>>
>> we have found that in our testing it seems to resolve problems flagged by
>> Travis. I think that if this is the way forwards then the code could
>> be collapsed back into netdev_vport_get_ifindex() and the surrounding
>> #ifdef __linux__ logic could be removed.
>>
>
> thanks Simon! looks good.
> We'll send V5 with your patches and some other fixes we did.
>
>

Hi Simon.

I'm not sure we can get rid of the ifdef __linux__ because of 
linux_get_ifindex(). Also we are actually missing another ifdef 
__linux__ around the include of netdev-linux.h.

Thanks,
Roi


>> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
>> index 4cc086e..d055d53 100644
>> --- a/lib/netdev-vport.c
>> +++ b/lib/netdev-vport.c
>> @@ -783,7 +783,7 @@ get_stats(const struct netdev *netdev, struct
>> netdev_stats *stats)
>>
>>  #ifdef __linux__
>>  static int
>> -netdev_vport_get_ifindex(const struct netdev *netdev_)
>> +netdev_vport_get_ifindex__(const struct netdev *netdev_)
>>  {
>>      char buf[NETDEV_VPORT_NAME_BUFSIZE];
>>      const char *name = netdev_vport_get_dpif_port(netdev_, buf,
>> sizeof(buf));
>> @@ -791,6 +791,15 @@ netdev_vport_get_ifindex(const struct netdev
>> *netdev_)
>>      return linux_get_ifindex(name);
>>  }
>>
>> +static int
>> +netdev_vport_get_ifindex(const struct netdev *netdev_)
>> +{
>> +    if (netdev_flow_api_enabled)
>> +        return netdev_vport_get_ifindex__(netdev_);
>> +    else
>> +        return -EOPNOTSUPP;
>> +}
>> +
>>  #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
>>  #define NETDEV_FLOW_OFFLOAD_API LINUX_FLOW_OFFLOAD_API
>>  #else /* !__linux__ */
>>
Simon Horman March 20, 2017, 11:44 a.m. UTC | #3 
On Mon, Mar 20, 2017 at 10:56:34AM +0200, Roi Dayan wrote:
> 
> 
> On 20/03/2017 10:33, Roi Dayan wrote:
> >
> >
> >On 16/03/2017 17:40, Simon Horman wrote:
> >>On Mon, Mar 13, 2017 at 03:36:50PM +0200, Roi Dayan wrote:
> >>>This patch series introduces rule offload functionality to dpif-netlink
> >>>via netdev ports new flow offloading API. The user can specify
> >>>whether to
> >>>enable rule offloading or not via OVS configuration. Netdev providers
> >>>are able to implement netdev flow offload API in order to offload rules.
> >>>
> >>>This patch series also implements one offload scheme for netdev-linux,
> >>>using TC flower classifier, which was chosen because its sort of natural
> >>>to state OVS DP rules for this classifier. However, the code can be
> >>>extended to support other classifiers such as U32, eBPF, etc which
> >>>support
> >>>offload as well.
> >>>
> >>>The use-case we are currently addressing is the newly sriov switchdev
> >>>mode
> >>>in the Linux kernel which was introduced in version 4.8 [1][2].
> >>>This series was tested against sriov vfs vports representors of the
> >>>Mellanox 100G ConnectX-4 series exposed by the mlx5 kernel driver.
> >>>
> >>>
> >>>V3->V4:
> >>>    - Move declarations to the right commit with implementation
> >>>    - Fix tc_get_flower flow return false success
> >>>    - Fix memory leaks - not releasing tc_transact replies
> >>>    - Fix travis failure for OSX compilation
> >>>    - Fix loop in dpif_netlink_flow_dump_next
> >>>    - Fix declared default value for tc-policy in vswitch.xml
> >>>    - Refactor loop in netdev_tc_flow_dump_next
> >>>    - Add missing error checks in parse_flow_put
> >>>    - Fix handling modify request where old rule is in hw and new
> >>>      rule is not supported and needs to be in sw.
> >>>    - Use 2 hashmaps instead of 1 for faster reverse lookup of ufid
> >>>from tc
> >>>    - Init ports when enabling hw-offload after OVS is running
> >>>
> >>>    TODO: Fix breaking of datapath compilation
> >>>          Fix testsuite failures
> >>>
> >>>    Travis
> >>>        https://travis-ci.org/roidayan/ovs/builds/210549325
> >>>    AppVeyor
> >>>        https://ci.appveyor.com/project/roidayan/ovs/build/1.0.15
> >>
> >>Hi Roi,
> >>
> >>we have found that in our testing it seems to resolve problems flagged by
> >>Travis. I think that if this is the way forwards then the code could
> >>be collapsed back into netdev_vport_get_ifindex() and the surrounding
> >>#ifdef __linux__ logic could be removed.
> >>
> >
> >thanks Simon! looks good.
> >We'll send V5 with your patches and some other fixes we did.
> >
> >
> 
> Hi Simon.
> 
> I'm not sure we can get rid of the ifdef __linux__ because of
> linux_get_ifindex(). Also we are actually missing another ifdef __linux__
> around the include of netdev-linux.h.

I'm with the __linux__ bits if they are still needed.
diff mbox

Patch

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 4cc086e..d055d53 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -783,7 +783,7 @@  get_stats(const struct netdev *netdev, struct netdev_stats *stats)
 
 #ifdef __linux__
 static int
-netdev_vport_get_ifindex(const struct netdev *netdev_)
+netdev_vport_get_ifindex__(const struct netdev *netdev_)
 {
     char buf[NETDEV_VPORT_NAME_BUFSIZE];
     const char *name = netdev_vport_get_dpif_port(netdev_, buf, sizeof(buf));
@@ -791,6 +791,15 @@  netdev_vport_get_ifindex(const struct netdev *netdev_)
     return linux_get_ifindex(name);
 }
 
+static int
+netdev_vport_get_ifindex(const struct netdev *netdev_)
+{
+    if (netdev_flow_api_enabled)
+        return netdev_vport_get_ifindex__(netdev_);
+    else
+        return -EOPNOTSUPP;
+}
+
 #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
 #define NETDEV_FLOW_OFFLOAD_API LINUX_FLOW_OFFLOAD_API
 #else /* !__linux__ */