On 31/05/2017 04:37, Joe Stringer wrote:
On 28 May 2017 at 04:59, Roi Dayan <r...@mellanox.com> wrote:
From: Paul Blakey <pa...@mellanox.com>
Add a new API interface for offloading dpif flows to netdev.
The API consist on the following:
flow_put - offload a new flow
flow_get - query an offloaded flow
flow_del - delete an offloaded flow
flow_flush - flush all offloaded flows
flow_dump_* - dump all offloaded flows
In upcoming commits we will introduce an implementation of this
API for netdev-linux.
Signed-off-by: Paul Blakey <pa...@mellanox.com>
Reviewed-by: Roi Dayan <r...@mellanox.com>
Reviewed-by: Simon Horman <simon.hor...@netronome.com>
---
<snip>
@@ -769,6 +777,67 @@ 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. */
+
+ /* Flush all offloaded flows from a netdev.
+ * Return 0 if successful, otherwise returns a positive errno value. */
+ int (*flow_flush)(struct netdev *);
+
+ /* Flow dumping interface.
+ *
+ * This is the back-end for the flow dumping interface described in
+ * dpif.h. Please read the comments there first, because this code
+ * closely follows it.
+ *
+ * 'flow_dump_create' is being executed in a dpif thread so there is
+ * no need for 'flow_dump_thread_create' implementation.
I find this comment a bit confusing, but it's a good thing it was here
because it raises a couple of discussion points.
'flow_dump_thread_create', perhaps poorly named, doesn't create a
thread, but allocates memory for per-thread state so that each thread
may dump safely in parallel while operating on an independent netlink
dump and independent buffers. I guess that in the DPIF flow dump there
is global dump state and per-thread state, while in this netdev flow
dump API there is only global state?
Describing that this interface doesn't need something that isn't being
defined is a bit strange. If it's not needed, then we probably don't
need to describe why it's not needed here since there's no such
function. Then, the comment can be dropped.
+ * On success returns allocated netdev_flow_dump data, on failure returns
^ returns allocated netdev_flow_dump_data "and returns 0"...?
+ * positive errno. */
+ int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump);
+ int (*flow_dump_destroy)(struct netdev_flow_dump *);
+
+ /* Returns true while there are more flows to dump.
s/while/if/
+ * rbuffer is used as a temporary buffer and needs to be pre allocated
+ * by the caller. while there are more flows the same rbuffer should
+ * be provided. wbuffer is used to store dumped actions and needs to be
+ * pre allocated by the caller. */
I have a couple of extra questions which this description could be
expanded to answer:
Who is responsible for freeing 'rbuffer' and 'wbuffer'? I expect the
caller, but this could be more explicit.
caller. as noted the function expects them to be pre allocated.
Are the pointers that are returned valid beyond the next call to
flow_dump_next()?
yes. what can we add to make it clear?
Please also capitalize the starts of sentences. (I'll say this once,
but it applies to several of the comments).
ok
+ 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);
+
+ /* Offload the given flow on netdev.
+ * To modify a flow, use the same ufid.
+ * actions are in netlink format, as with struct dpif_flow_put.
Is this "OVS netlink format" or "TC flower netlink format"?
netlink
+ * info is extra info needed to offload the flow.
+ * Read the comments on 'struct dpif_flow_put' in dpif.h about stats.
This sentence about stats is more descriptive if it states something such as:
'stats' is populated according to the rules set out in the description
above 'struct dpif_flow_del'.
ok
+ * Return 0 if successful, otherwise returns a positive errno value. */
+ int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions,
+ size_t actions_len, const ovs_u128 *ufid,
+ struct offload_info *info, struct dpif_flow_stats *);
+
+ /* Queries a flow specified by ufid on netdev.
+ * Fills output buffer as wbuffer in flow_dump_next.
+ * the buffer needs to be pre allocated.
+ * Return 0 if successful, otherwise returns a positive errno value. */
How is the caller expected to use the parameters? If it is expected to
use the buffer and interpret its data, that should be described. If
not, then 'buffer' should be described as temporary storage for
fetching the requested flow, and the other parameters should be
described in more detail. The state of each pointer should also be
described depending on the success or failure of the function.
ok
Put another way, what are the input and output parameters?
output params are stats and wbuffer.
Looking around, there's still several parameters left undefined in the
APIs throughout this patch. Please also look at the others.
ok
+ int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions,
+ const ovs_u128 *ufid, struct dpif_flow_stats *,
+ struct ofpbuf *wbuffer);
+
+ /* Delete a flow specified by ufid from netdev.
+ * Read the comments on 'struct dpif_flow_del' in dpif.h about stats.
Same stats comment as flow_put().
+ * Return 0 if successful, otherwise returns a positive errno value. */
+ int (*flow_del)(struct netdev *, const ovs_u128 *ufid,
+ struct dpif_flow_stats *);
+
+ /* Initializies the netdev flow api. */
It seems that you missed the description of the return code in the
comment above.
ok
+ int (*init_flow_api)(struct netdev *);
};
int netdev_register_provider(const struct netdev_class *);
<snip>
diff --git a/lib/netdev.h b/lib/netdev.h
index d6c07c1..17890e6 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 */
+};
I see that the port_hmap still isn't defined as an actual type. If you
don't mind refreshing my memory, was there a hurdle in defining this
more formally?
I plan on stopping here with my review for now.
Hi Joe,
I accidentally skipped your comments here for V10.
I'll address them in the next update.
We skipped addressing port_hmap_obj as we also wanted to move it from
global to be per dpif which I think got me stuck somewhere in the
process. I don't remember the reason.
maybe we can still do as a first step changing this void* to some
type but still be global and later to be per dpif.
in any case can we address this in a later commit ?
Thanks,
Roi
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
