Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows

[Simon Horman]

On Wed, May 03, 2017 at 08:20:18AM +0300, Roi Dayan wrote:
> 
> 
> On 24/04/2017 14:35, Simon Horman wrote:
> >On Tue, Apr 18, 2017 at 03:18:55PM +0300, Roi Dayan wrote:
> >>
> >>
> >>On 14/04/2017 04:11, Joe Stringer wrote:
> >>>On 7 April 2017 at 06:12, Roi Dayan  wrote:
> From: Paul Blakey 
> 
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> Reviewed-by: Simon Horman 
> ---
> >>>
> >>>
> >>>
> diff --git a/lib/netdev.h b/lib/netdev.h
> index d6c07c1..6d2db7d 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct 
> dp_packet_batch *,
> bool may_steal, bool concurrent_txq);
> void netdev_send_wait(struct netdev *, int qid);
> 
> +/* Flow offloading. */
> +struct offload_info {
> +const void *port_hmap_obj; /* To query ports info from netdev port 
> map */
> +ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */
> >>>
> >>>Is this assuming there is only ever one tunnel destination port? What
> >>>about multiple output?
> >>>
> >>
> >>Yes. we currently only support single dst port output. if there is more than
> >>1 we fail with EOPNOTSUPP and fallback to OVS datapath.
> >>the check is done in dpif-netlink.c parse_flow_put()
> >
> >FWIW, from my PoV this is a shortcoming of the feature-set provided by
> >this patch-set and is something I would like to see lifted at some point in
> >the future - I would be interested in doing so if it isn't on anyone else's
> >todo list. Perhaps it would be useful to formally list what offloads are
> >supported by the API?
> >
> >The above notwithstanding, I would expect that output to a single tunnel
> >vport covers a lot of use-cases.
> >
> 
> Yes, we do want to support multiple destinations and considered it as a
> later feature which is on our TODO, though we haven't started working on
> this yet.
> As for a formal list of what is offloads are supported, I guess we can list
> it later in the documentation.

That would be fine by me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows

[Roi Dayan]

On 24/04/2017 14:35, Simon Horman wrote:

On Tue, Apr 18, 2017 at 03:18:55PM +0300, Roi Dayan wrote:



On 14/04/2017 04:11, Joe Stringer wrote:

On 7 April 2017 at 06:12, Roi Dayan  wrote:

From: Paul Blakey 

Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
Reviewed-by: Simon Horman 
---





diff --git a/lib/netdev.h b/lib/netdev.h
index d6c07c1..6d2db7d 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct 
dp_packet_batch *,
bool may_steal, bool concurrent_txq);
void netdev_send_wait(struct netdev *, int qid);

+/* Flow offloading. */
+struct offload_info {
+const void *port_hmap_obj; /* To query ports info from netdev port map */
+ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */


Is this assuming there is only ever one tunnel destination port? What
about multiple output?



Yes. we currently only support single dst port output. if there is more than
1 we fail with EOPNOTSUPP and fallback to OVS datapath.
the check is done in dpif-netlink.c parse_flow_put()


FWIW, from my PoV this is a shortcoming of the feature-set provided by
this patch-set and is something I would like to see lifted at some point in
the future - I would be interested in doing so if it isn't on anyone else's
todo list. Perhaps it would be useful to formally list what offloads are
supported by the API?

The above notwithstanding, I would expect that output to a single tunnel
vport covers a lot of use-cases.



Yes, we do want to support multiple destinations and considered it as a 
later feature which is on our TODO, though we haven't started working on 
this yet.
As for a formal list of what is offloads are supported, I guess we can 
list it later in the documentation.

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

Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows

[Simon Horman]

On Tue, Apr 18, 2017 at 03:18:55PM +0300, Roi Dayan wrote:
> 
> 
> On 14/04/2017 04:11, Joe Stringer wrote:
> >On 7 April 2017 at 06:12, Roi Dayan  wrote:
> >>From: Paul Blakey 
> >>
> >>Signed-off-by: Paul Blakey 
> >>Reviewed-by: Roi Dayan 
> >>Reviewed-by: Simon Horman 
> >>---
> >
> >
> >
> >>diff --git a/lib/netdev.h b/lib/netdev.h
> >>index d6c07c1..6d2db7d 100644
> >>--- a/lib/netdev.h
> >>+++ b/lib/netdev.h
> >>@@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct 
> >>dp_packet_batch *,
> >> bool may_steal, bool concurrent_txq);
> >> void netdev_send_wait(struct netdev *, int qid);
> >>
> >>+/* Flow offloading. */
> >>+struct offload_info {
> >>+const void *port_hmap_obj; /* To query ports info from netdev port map 
> >>*/
> >>+ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */
> >
> >Is this assuming there is only ever one tunnel destination port? What
> >about multiple output?
> >
> 
> Yes. we currently only support single dst port output. if there is more than
> 1 we fail with EOPNOTSUPP and fallback to OVS datapath.
> the check is done in dpif-netlink.c parse_flow_put()

FWIW, from my PoV this is a shortcoming of the feature-set provided by
this patch-set and is something I would like to see lifted at some point in
the future - I would be interested in doing so if it isn't on anyone else's
todo list. Perhaps it would be useful to formally list what offloads are
supported by the API?

The above notwithstanding, I would expect that output to a single tunnel
vport covers a lot of use-cases.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows

[Roi Dayan]

On 14/04/2017 04:11, Joe Stringer wrote:

On 7 April 2017 at 06:12, Roi Dayan  wrote:

From: Paul Blakey 

Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
Reviewed-by: Simon Horman 
---





diff --git a/lib/netdev.h b/lib/netdev.h
index d6c07c1..6d2db7d 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct 
dp_packet_batch *,
 bool may_steal, bool concurrent_txq);
 void netdev_send_wait(struct netdev *, int qid);

+/* Flow offloading. */
+struct offload_info {
+const void *port_hmap_obj; /* To query ports info from netdev port map */
+ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */


Is this assuming there is only ever one tunnel destination port? What
about multiple output?



Yes. we currently only support single dst port output. if there is more 
than 1 we fail with EOPNOTSUPP and fallback to OVS datapath.

the check is done in dpif-netlink.c parse_flow_put()

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

Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows

[Joe Stringer]

On 7 April 2017 at 06:12, Roi Dayan  wrote:
> From: Paul Blakey 
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> Reviewed-by: Simon Horman 
> ---



> diff --git a/lib/netdev.h b/lib/netdev.h
> index d6c07c1..6d2db7d 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct 
> dp_packet_batch *,
>  bool may_steal, bool concurrent_txq);
>  void netdev_send_wait(struct netdev *, int qid);
>
> +/* Flow offloading. */
> +struct offload_info {
> +const void *port_hmap_obj; /* To query ports info from netdev port map */
> +ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */

Is this assuming there is only ever one tunnel destination port? What
about multiple output?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows

[Joe Stringer]

On 7 April 2017 at 06:12, Roi Dayan  wrote:
> From: Paul Blakey 
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> Reviewed-by: Simon Horman 
> ---



> @@ -769,6 +777,49 @@ struct netdev_class {
>
>  /* Discards all packets waiting to be received from 'rx'. */
>  int (*rxq_drain)(struct netdev_rxq *rx);
> +
> +/* ##  ## */
> +/* ## netdev flow offloading functions ## */
> +/* ##  ## */
> +
> +/* If a particular netdev class does not support offloading flows, all these
> + * function pointers must be NULL. */
> +
> +/* Deleting all offloaded flows from netdev */
> +int (*flow_flush)(struct netdev *);
> +/* Dumping interface:
> + * Usage is as with dpif_port_dump api (create, next, destory).
> + * Create sets dump on success or returns error status on failure. */

What is the error status? negative? positive? errno?

Common OVS descriptions say something like:

Returns ___, if successful. On failure, returns a negative errno value.

> +int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump);
> +int (*flow_dump_destroy)(struct netdev_flow_dump *);
> + /* rbuffer is for use of the implementation (e.g using nl_dump),
> + * and is usually shared for the given thread that runs flow_dump_next.
> + * wbuffer is the buffer that dumped actions will be stored in, and given
> + * pointers to. */
> +bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *,
> +   struct nlattr **actions,
> +   struct dpif_flow_stats *stats, ovs_u128 *ufid,
> +   struct ofpbuf *rbuffer, struct ofpbuf *wbuffer);

How does this function expect 'rbuffer' and 'wbuffer' to be prepared?
Are they already allocated? Will this function reallocate them? Who
frees them?

What does this function return?

I think that the equivalent dpif interface describes that the
parameters to flow_dump_next() must be prepared by flow_dump_create().

> +
> +/* Offload the given flow (match, actions, stats, ufid) on netdev.
> + * If stats isn't null, sets the given stats for that flow.
> + * To modify the flow, use the same ufid.
> + * actions are in netlink format, as with struct dpif_flow_put.
> + * info is anything else that is need to offload the flow. */
> +int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions,
> +size_t actions_len, struct dpif_flow_stats *,
> +const ovs_u128 *ufid, struct offload_info *info);

Really, it sets the stats? Does it ever retrieve stats (for example,
if you modify the flow)?

What does it return?

> +/* Queries the flow with specified ufid on netdev.
> + * Fills match, actions, stats as with flow_dump_next */
> +int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions,
> +struct dpif_flow_stats *, const ovs_u128 *ufid,
> +struct ofpbuf *);
> +/* Deletes the given flow specified by ufid from netdev.
> + * If stats is not null, fills it with flow stats. */
> +int (*flow_del)(struct netdev *, const ovs_u128 *ufid,
> +struct dpif_flow_stats *);
> +/* Initializies the netdev flow api. */
> +int (*init_flow_api)(struct netdev *);
>  };

Hopefully you're getting a sense for the set of questions I'm asking
around how you describe this API, and can apply that feedback to the
rest of these as well.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows

[Roi Dayan]

From: Paul Blakey 

Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
Reviewed-by: Simon Horman 
---
 lib/automake.mk  |   2 +
 lib/netdev-bsd.c |   2 +
 lib/netdev-dpdk.c|   1 +
 lib/netdev-dummy.c   |   2 +
 lib/netdev-linux.c   |  15 +++--
 lib/netdev-linux.h   |   9 +++
 lib/netdev-provider.h|  53 
 lib/netdev-tc-offloads.c | 154 +++
 lib/netdev-tc-offloads.h |  42 +
 lib/netdev-vport.c   |  11 +++-
 lib/netdev.c |  98 ++
 lib/netdev.h |  23 +++
 12 files changed, 407 insertions(+), 5 deletions(-)
 create mode 100644 lib/netdev-tc-offloads.c
 create mode 100644 lib/netdev-tc-offloads.h

diff --git a/lib/automake.mk b/lib/automake.mk
index 7c1b67b..6e7e8ee 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -354,6 +354,8 @@ lib_libopenvswitch_la_SOURCES += \
lib/dpif-netlink.h \
lib/tc.h \
lib/tc.c \
+   lib/netdev-tc-offloads.h \
+   lib/netdev-tc-offloads.c \
lib/if-notifier.c \
lib/if-notifier.h \
lib/netdev-linux.c \
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 94c515d..5b54d79 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1547,6 +1547,8 @@ netdev_bsd_update_flags(struct netdev *netdev_, enum 
netdev_flags off,
 netdev_bsd_rxq_recv, \
 netdev_bsd_rxq_wait, \
 netdev_bsd_rxq_drain,\
+ \
+NO_OFFLOAD_API   \
 }
 
 const struct netdev_class netdev_bsd_class =
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ddc651b..9ad96cd 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -3314,6 +3314,7 @@ unlock:
 RXQ_RECV, \
 NULL,   /* rx_wait */ \
 NULL,   /* rxq_drain */   \
+NO_OFFLOAD_API\
 }
 
 static const struct netdev_class dpdk_class =
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 0657434..7e1383f 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1409,6 +1409,8 @@ netdev_dummy_update_flags(struct netdev *netdev_,
 netdev_dummy_rxq_recv,  \
 netdev_dummy_rxq_wait,  \
 netdev_dummy_rxq_drain, \
+\
+NO_OFFLOAD_API  \
 }
 
 static const struct netdev_class dummy_class =
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 085f530..0828d96 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -73,6 +73,7 @@
 #include "openvswitch/vlog.h"
 #include "util.h"
 #include "tc.h"
+#include "netdev-tc-offloads.h"
 
 VLOG_DEFINE_THIS_MODULE(netdev_linux);
 
@@ -2762,7 +2763,8 @@ netdev_linux_update_flags(struct netdev *netdev_, enum 
netdev_flags off,
 }
 
 #define NETDEV_LINUX_CLASS(NAME, CONSTRUCT, GET_STATS,  \
-   GET_FEATURES, GET_STATUS)\
+   GET_FEATURES, GET_STATUS,\
+   FLOW_OFFLOAD_API)\
 {   \
 NAME,   \
 false,  /* is_pmd */\
@@ -2831,6 +2833,8 @@ netdev_linux_update_flags(struct netdev *netdev_, enum 
netdev_flags off,
 netdev_linux_rxq_recv,  \
 netdev_linux_rxq_wait,  \
 netdev_linux_rxq_drain, \
+\
+FLOW_OFFLOAD_API\
 }
 
 const struct netdev_class netdev_linux_class =
@@ -2839,7 +2843,8 @@ const struct netdev_class netdev_linux_class =
 netdev_linux_construct,
 netdev_linux_get_stats,
 netdev_linux_get_features,
-netdev_linux_get_status);
+netdev_linux_get_status,
+LINUX_FLOW_OFFLOAD_API);
 
 const struct netdev_class netdev_tap_class =
 NETDEV_LINUX_CLASS(
@@ -2847,7 +2852,8 @@ const struct netdev_class netdev_tap_class =
 netdev_linux_construct_tap,
 netdev_tap_get_stats,
 netdev_linux_get_features,
-netdev_linux_get_status);
+netdev_linux_get_status,
+NO_OFFLOAD_API);
 
 const struct netdev_class netdev_internal_class =
 NETDEV_LINUX_CLASS(
@@ -2855,7 +2861,8 @@ const struct netdev_class netdev_internal_class =