On Mon, Jul 18, 2016 at 03:15:13PM +0900, Simon Horman wrote:
> Hi Johnson,
>
> On Wed, Jul 13, 2016 at 01:28:48AM +0800, Johnson Li wrote:
> > Signed-off-by: Johnson Li <johnson...@intel.com>
>
> * 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.
Simon, very good guide, do push_eth and pop_eth also need to follow
this?
>
> > 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
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
