[ovs-dev] [patch_v8] ovn: Fix receive from vxlan in ovn-controller.

Darrell Ball dlu998 at gmail.com
Thu Aug 11 00:29:25 UTC 2016


On Wed, Aug 10, 2016 at 4:33 PM, Justin Pettit <jpettit at ovn.org> wrote:

>
> > On Aug 8, 2016, at 7:20 PM, Darrell Ball <dlu998 at gmail.com> wrote:
> >
> >
> > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> > index 589b053..43885fd 100644
> > --- a/ovn/controller/physical.c
> > +++ b/ovn/controller/physical.c
> > @@ -496,6 +496,21 @@ consider_port_binding(enum mf_field_id
> mff_ovn_geneve,
> >         ofpact_put_OUTPUT(ofpacts_p)->port = ofport;
> >         ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 100,
> >                         &match, ofpacts_p, &binding->header_.uuid);
> > +
> > +        /* For packets received from a Vxlan tunnel which get
> > +         * resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to lack of
> > +         * needed metadata in Vxlan, explicitly skip sending back out
> > +         * any tunnels and resubmit to table 33 for local delivery. */
>
> The capitalization of "VXLAN" is inconsistent in this comment.  I would
> just use "VXLAN", since that's how it is in most of the code base.
>

I checked the code and the capitalization used is indeed all uppercase.



>
> I'd move this comment to the big comment describing "Table 32, priority
> 100", and adding this priority to that heading.
>
> > +         match_init_catchall(&match);
> > +         ofpbuf_clear(ofpacts_p);
> > +         match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
> > +                              MLF_RCV_FROM_VXLAN,
> > +                              MLF_RCV_FROM_VXLAN);
> > +         /* Resubmit to table 33. */
> > +         put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
> > +         ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 101, &match, ofpacts_p,
> > +                         &binding->header_.uuid);
>
> This code is indented to 9 columns instead of 8.
>
> It seems like we've usually used a priority of 150 for this higher
> priority flow.
>

There was one prior art case of using 150 and 100, rather than 101 and 100
in the
file.



>
> > diff --git a/ovn/lib/logical-fields.h b/ovn/lib/logical-fields.h
> > index 1ea2e0f..1053c86 100644
> > --- a/ovn/lib/logical-fields.h
> > +++ b/ovn/lib/logical-fields.h
> >
> > @@ -52,4 +43,21 @@ enum {
> >
> > void ovn_init_symtab(struct shash *symtab);
> >
> > +/* MFF_LOG_FLAGS_REG bit assignments */
> > +enum mff_log_flags_bits {
> > +    MLF_ALLOW_LOOPBACK_BIT = 0,
> > +    MLF_RCV_FROM_VXLAN_BIT = 1,
> > +};
> > +
> > +/* MFF_LOG_FLAGS_REG flag assignments */
> > +enum mff_log_flags {
> > +    MLF_ALLOW_LOOPBACK = (1 << MLF_ALLOW_LOOPBACK_BIT), /* Allow
> outputting
> > +                                                          back to
> inport. */
> > +    /* The below flag is used to indicate that a packet was received
> from a
> > +     * vxlan tunnel to compensate for the lack of egress port
> information
> > +     * available in Vxlan encapsulation.  Egress port information is
> > +     * available for Geneve and STT tunnel types. */
> > +    MLF_RCV_FROM_VXLAN = (1 << MLF_RCV_FROM_VXLAN_BIT),
>
> The capitalization of "VXLAN" is inconsistent in this comment, too.  I'd
> also change the comment describing MLF_ALLOW_LOOPBACK to be right above the
> definition so that it's consistent with the big comment you've added.
>

yep, VXLAN should be all uppercase



>
> I've attached a diff that contains my suggestions (along with a couple
> other minor things).  If you're happy with it, I'll commit the patch.
>


I commented below - they are fine.

Thanks Darrell



>
> Thanks,
>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-
>
>
> diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
> index b2086ad..a72b149 100644
> --- a/ovn/controller-vtep/vtep.c
> +++ b/ovn/controller-vtep/vtep.c
> @@ -231,6 +231,7 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset
> *vtep_pswi
>                           vtep_ls->tunnel_key[0], tnl_key);
>              }
>              vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
> +
>              /* OVN is expected to always use source node replication mode,
>               * hence the replication mode is hard-coded for each logical
>               * switch in the context of ovn-controller-vtep. */
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 43885fd..4448756 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -474,13 +474,31 @@ consider_port_binding(enum mf_field_id
> mff_ovn_geneve,
>                          &match, ofpacts_p, &binding->header_.uuid);
>      } else {
>          /* Remote port connected by tunnel */
> -        /* Table 32, priority 100.
> -         * =======================
> +
> +        /* Table 32, priority 150 and 100.
> +         * ===============================
>           *
> -         * Implements output to remote hypervisors.  Each flow matches an
> -         * output port that includes a logical port on a remote
> hypervisor,
> -         * and tunnels the packet to that hypervisor.
> +         * Priority 150 is for packets received from a VXLAN tunnel
> +         * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
> +         * lack of needed metadata in VXLAN, explicitly skip sending
> +         * back out any tunnels and resubmit to table 33 for local
> +         * delivery.
> +         *
>

Moving the comment in front of the previous comment for pre-existing lower
priority 100
flows is fine.


> +         * Priority 100 is for all other traffic which need to be sent
> +         * to a remote hypervisor.  Each flow matches an output port
> +         * that includes a logical port on a remote hypervisor, and
> +         * tunnels the packet to that hypervisor. */


You co-located the comments for pre-existing and new flows before the
associated code - ok.



> +        match_init_catchall(&match);
> +        ofpbuf_clear(ofpacts_p);
> +        match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
> +                             MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
> +
> +        /* Resubmit to table 33. */
> +        put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
> +        ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 150, &match, ofpacts_p,
> +                        &binding->header_.uuid);
> +
>

Moving the code next to the Table 32 comment is fine.




>
>          match_init_catchall(&match);
>          ofpbuf_clear(ofpacts_p);
> @@ -496,21 +514,6 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
>          ofpact_put_OUTPUT(ofpacts_p)->port = ofport;
>          ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 100,
>                          &match, ofpacts_p, &binding->header_.uuid);
> -
> -        /* For packets received from a Vxlan tunnel which get
> -         * resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to lack of
> -         * needed metadata in Vxlan, explicitly skip sending back out
> -         * any tunnels and resubmit to table 33 for local delivery. */
> -         match_init_catchall(&match);
> -         ofpbuf_clear(ofpacts_p);
> -         match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
> -                              MLF_RCV_FROM_VXLAN,
> -                              MLF_RCV_FROM_VXLAN);
> -         /* Resubmit to table 33. */
> -         put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
> -         ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 101, &match, ofpacts_p,
> -                         &binding->header_.uuid);
> -
>

Keeping code and comments together is fine.



>      }
>  }
>
> diff --git a/ovn/lib/logical-fields.h b/ovn/lib/logical-fields.h
> index 1053c86..a1f1da6 100644
> --- a/ovn/lib/logical-fields.h
> +++ b/ovn/lib/logical-fields.h
> @@ -51,12 +51,13 @@ enum mff_log_flags_bits {
>
>  /* MFF_LOG_FLAGS_REG flag assignments */
>  enum mff_log_flags {
> -    MLF_ALLOW_LOOPBACK = (1 << MLF_ALLOW_LOOPBACK_BIT), /* Allow
> outputting
> -                                                          back to inport.
> */
> -    /* The below flag is used to indicate that a packet was received from
> a
> -     * vxlan tunnel to compensate for the lack of egress port information
> -     * available in Vxlan encapsulation.  Egress port information is
> -     * available for Geneve and STT tunnel types. */
> +    /* Allow outputting back to inport. */
>

I thought about moving the comment about the pre-existing LOOPBACK flag in
front of the code but was not sure about preferred art regarding placement
of comments.


> +    MLF_ALLOW_LOOPBACK = (1 << MLF_ALLOW_LOOPBACK_BIT),
> +
> +    /* Indicate that a packet was received from a VXLAN tunnel to
> +     * compensate for the lack of egress port information available in
> +     * VXLAN encapsulation.  Egress port information is available for
> +     * Geneve and STT tunnel types. */
>      MLF_RCV_FROM_VXLAN = (1 << MLF_RCV_FROM_VXLAN_BIT),
>  };
>
>
>



More information about the dev mailing list