[ovs-dev] [RFC PATCH v2 02/13] Format NSH keys to readable strings

[Yang, Yi]

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.li at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev