| Message ID | BE1F6398-6E9E-4673-A502-0B4CFEB46C28@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
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), > }; > > >
> 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
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), };