Hi Johnson,
On Wed, Jul 13, 2016 at 01:28:48AM +0800, Johnson Li wrote:
> Signed-off-by: Johnson Li <[email protected]>
* Regarding the action set (which we discussed briefly off-list):
I think that you need to update ofpacts_execute_action_set(), though
possibly not in this patch, for push/pop_nsh to be usable in write
actions. However, its not clear to me at which position of that function
push/pop_nsh because as far as I know this has not been defined by
OpenFlow.
* Regarding the implementation of push/pop_nsh in this patch:
In general translation occurs in two phases in OvS.
1. Composition: This generally involves updating fields of
ctx->xin->flow to new desired values and ctx->wc to indicate
which fields have been accessed so that masks for megaflows
can be calculated correctly.
For simple cases such as OFPACT_SET_IP_TTL this is open-coded
in do_xlate_actions. For more complex cases helpers are provided,
e.g. OFPACT_PUSH_MPLS (though that is not very complex)
2. Commit: Here differences between ctx->in->flow and ctx->base_flow,
which are the same before translation starts, are compared. And any
differences are resolved by writing actions to ctx->odp_actions.
base_flow is then reset for cases where translation continues.
This is performed by commit_odp_actions(), e.g. when called via
xlate_commit_actions().
There are exceptions to the above and in some cases actions are written
directly to ctx->odp_actions, but I'm not sure that push/pop_nsh needs to
be such an exception.
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 90edb56..1a63175 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5072,8 +5072,58 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> ctx_trigger_freeze(ctx);
> a = ofpact_next(a);
> break;
> - case OFPACT_PUSH_NSH:
> + case OFPACT_PUSH_NSH: {
> + struct ovs_action_push_nsh push;
> + struct nsh_header *nsh = (struct nsh_header *)push.header;
I think OvS prefers, though admittedly does not strictly follow,
the reverse christmas-tree (longest to shortest line) ordering of
variables. So the above two lines should be inverted.
> + //int md_len = 0;
> + //bool crit_opt;
Please remove commented out code.
> +
> + if (flow->nsh.md_type == NSH_MD_TYPE1) {
> + struct nsh_md1_ctx *ctx = NULL;
> +
> + nsh->base.flags = flow->nsh.flags;
> + nsh->base.length = NSH_TYPE1_LEN >> 2;
> + nsh->base.md_type = NSH_MD_TYPE1;
> + nsh->base.next_proto = flow->nsh.next_proto;
> + nsh->base.sfp = (flow->nsh.nsp >> 8) | (flow->nsh.nsi << 24);
> +
> + ctx = (struct nsh_md1_ctx *)(nsh + 1);
> + ctx->nshc1 = flow->nsh.nshc1;
> + ctx->nshc2 = flow->nsh.nshc2;
> + ctx->nshc3 = flow->nsh.nshc3;
> + ctx->nshc4 = flow->nsh.nshc4;
> +
> + push.len = NSH_TYPE1_LEN;
> +#if 0
Please remove #if 0. If the enclosed code is not as ready as the rest of
the patch please remove it too.
> + } else if (flow->nsh.md_type == NSH_MD_TYPE2) {
> + /* MD type 2 prototype with TUN_METADATA APIs. */
> + struct nsh_md2_ctx *ctx = NULL;
> +
> + nsh->base.md_type = NSH_MD_TYPE2;
> + nsh->base.next_proto = flow->nsh.next_proto;
> + nsh->base.sfp = (flow->nsh.nsp >> 8) | (flow->nsh.nsi << 24);
> +
> + ctx = (struct nsh_md2_ctx *)(nsh + 1);
> + md_len = tun_metadata_to_geneve_header(&flow->tunnel,
> + (struct geneve_opt
> *)ctx,
> + &crit_opt);
Perhaps it would be worth considering renaming this and other related
'geneve_' functions if they are being used beyond Geneve.
> + nsh->base.length = (sizeof(struct nsh_header) + md_len) >> 2;
> + push.len = md_len + sizeof(struct nsh_header);
> +#endif
> + }
Are we sure no other value of flow->nsh.md_type can be present?
> +
> + nl_msg_put_unspec(ctx->odp_actions, OVS_ACTION_ATTR_PUSH_NSH,
> + &push, sizeof push);
> +
> + flow->dl_type = htons(ETH_TYPE_NSH);
> + memset(&wc->masks.nsh.md_type, 0xff, sizeof
> wc->masks.nsh.md_type);
> + break;
> + }
> +
> case OFPACT_POP_NSH:
> + memset(&wc->masks.nsh.md_type, 0xff, sizeof
> wc->masks.nsh.md_type);
> + memset(&flow->nsh, 0x0, sizeof flow->nsh);
> + nl_msg_put_flag(ctx->odp_actions, OVS_ACTION_ATTR_POP_NSH);
> break;
> }
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
