[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