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.
Are the pointers that are returned valid beyond the next call to
flow_dump_next()?
Please also capitalize the starts of sentences. (I'll say this once,
but it applies to several of the comments).
> + 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"?
> + * 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'.
> + * 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.
Put another way, what are the input and output parameters?
Looking around, there's still several parameters left undefined in the
APIs throughout this patch. Please also look at the others.
> + 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.
> + 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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
