Jesse,
This is no proper review, but I have two questions below.
Jarno
> On Jul 7, 2015, at 10:30 PM, Jesse Gross <je...@nicira.com> wrote:
>
> The kernel implementation of Geneve options stores the TLV option
> data in the flow exactly as received, without any further parsing.
> This is then translated to known options for the purposes of matching
> on flow setup (which will then install a datapath flow in the form
> the kernel is expecting).
>
> The userspace implementation behaves a little bit differently - it
> looks up known options as each packet is received. The reason for this
> is there is a much tighter coupling between datapath and flow translation
> and the representation is generally expected to be the same. This works
> but it incurs work on a per-packet basis that could be done per-flow
> instead.
>
> This introduces a small translation step for Geneve packets between
> datapath and flow lookup for the userspace datapath in order to
> allow the same kind of processing that the kernel does.
>
> Signed-off-by: Jesse Gross <je...@nicira.com>
> ---
> lib/automake.mk | 1 +
> lib/dpif-netdev.c | 78 +++++++++++++++++++++-
> lib/flow.c | 23 +++++--
> lib/geneve.h | 63 ++++++++++++++++++
> lib/netdev-vport.c | 23 +++----
> lib/odp-util.c | 24 ++++---
> lib/odp-util.h | 6 ++
> lib/packets.h | 41 +-----------
> lib/tun-metadata.c | 166 ++++++++++++++++++++++++++++++-----------------
> lib/tun-metadata.h | 39 +++++++++--
> tests/tunnel-push-pop.at | 2 +-
> 11 files changed, 327 insertions(+), 139 deletions(-)
> create mode 100644 lib/geneve.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index f72eb32..dae3fee 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -81,6 +81,7 @@ lib_libopenvswitch_la_SOURCES = \
> lib/fatal-signal.h \
> lib/flow.c \
> lib/flow.h \
> + lib/geneve.h \
> lib/guarded-list.c \
> lib/guarded-list.h \
> lib/hash.c \
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index b53d52a..50763bc 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1830,6 +1830,7 @@ dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow
> *netdev_flow,
> .flow = &netdev_flow->flow,
> .mask = &wc.masks,
> .support = dp_netdev_support,
> + .udpif_no_xlate = true,
> };
>
> miniflow_expand(&netdev_flow->cr.mask->mf, &wc.masks);
> @@ -1861,6 +1862,23 @@ dp_netdev_flow_to_dpif_flow(const struct
> dp_netdev_flow *netdev_flow,
> get_dpif_flow_stats(netdev_flow, &flow->stats);
> }
>
> +static void
> +xlate_geneve_attr(const struct nlattr *key, uint32_t key_len,
> + bool is_mask, struct flow *flow)
> +{
> + const struct nlattr *geneve_key;
> +
> + /* We don't actually want any translation for Geneve - just copy the
> + * original over. */
> + memset(&flow->tunnel.metadata, 0, sizeof flow->tunnel.metadata);
Could we avoid zeroing the whole thing in the geneve case?
> + geneve_key = tun_metadata_find_geneve_key(key, key_len);
> + if (geneve_key) {
> + int len = nl_attr_get_size(geneve_key);
> + memcpy(&flow->tunnel.metadata.opts.gnv, nl_attr_get(geneve_key),
> len);
> + flow->tunnel.metadata.present.len = !is_mask ? len : 0xff;
Why the len is 0xff for the is_mask case?
> + }
> +}
> +
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
