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

Message ID BE1F6398-6E9E-4673-A502-0B4CFEB46C28@ovn.org
State Accepted
Headers show

Commit Message

Justin Pettit Aug. 10, 2016, 11:33 p.m. UTC
> On Aug 8, 2016, at 7:20 PM, Darrell Ball <dlu998@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'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.

> 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.

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.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-

Comments

Darrell Ball Aug. 11, 2016, 12:29 a.m. UTC | #1
On Wed, Aug 10, 2016 at 4:33 PM, Justin Pettit <jpettit@ovn.org> wrote:

>
> > On Aug 8, 2016, at 7:20 PM, Darrell Ball <dlu998@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),
>  };
>
>
>
Justin Pettit Aug. 11, 2016, 12:32 a.m. UTC | #2
> On Aug 10, 2016, at 5:29 PM, Darrell Ball <dlu998@gmail.com> wrote:
> 
> I commented below - they are fine.

Great.  I pushed it to master.  Thanks for fixing this long-standing issue.

--Justin

Patch
diff mbox

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.
+         *
+         * 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.
          */
+        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);
+
 
         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);
-
     }
 }
 
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. */
+    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),
 };