[ovs-discuss] [PATCH] ofproto-dpif: Add comments for a few VLAN splinters functions.

[Ben Pfaff]

On Tue, Mar 13, 2012 at 01:12:57PM +0800, Min Chen wrote:
> Thank you Ben for your helpful comments. 

Thanks, I pushed the patch.

> But a few more questions on standard MAC learning switch
> implementation in OVS:

Do you actually have questions about that?  So far, your questions
seem to be about OFPP_FLOOD, which is not the same thing.

> Take this for example:
> http://openvswitch.org/support/config-cookbooks/vlan-configuration-cookbook/
> 
> This is what's confused me: The *first* packet from VM1 to VM3 will
> cause(due to unknown MAC address) a PACKET_OUT with action OUTPUT:FLOOD to
> be sent from ovs-controller to the OVS on Host1.

I'm sure that's true.  Do you have the misconception that
ovs-controller is some kind of reference implementation of a
controller?  It isn't.  ovs-controller isn't a very good controller.
It isn't meant to be.  It's just a test program.

> OVS will enumerate all the
> ports to decide which ports to flood this packet to(to avoid the VLAN
> splinters feature, I am now reading Open vSwitch *1.3.0* source code)

OK.

> ofproto-dpif.c:
> 3884 static void
> 3885 flood_packets(struct action_xlate_ctx *ctx, ovs_be32 mask)
> 3886 {
> 3887     struct ofport_dpif *ofport;
> 3888
> 3889     commit_odp_actions(ctx);
> 3890     HMAP_FOR_EACH (ofport, up.hmap_node, &ctx->ofproto->up.ports) {
> 3891         uint16_t ofp_port = ofport->up.ofp_port;
> 3892         if (ofp_port != ctx->flow.in_port
> 3893                 && !(ofport->up.opp.config & mask)
> 3894                 && stp_forward_in_state(ofport->stp_state)) {
> 3895             compose_output_action(ctx, ofport->odp_port);
> 3896         }
> 3897     }
> 3898
> 3899     ctx->nf_output_iface = NF_OUT_FLOOD;
> 3900 }
> 
> On a standard VLAN-enabled swtich, this packet should not be flooded to
> port that's not in the same VLAN as VM1. Thus the packet should not be
> flooded to VM2(even though VM1 and VM2 are connected to the same OVS). But
> from the code above, I can't see any "if conditions" that will exclude
> this.

ovs-controller doesn't implement a VLAN-aware switch.  There is no way
to tell it which ports should be on which VMW.  (It does do MAC
learning on a per-VLAN basis, so I guess that you could regard it as a
VLAN-aware switch in which every port is a trunk port.)

(If ovs-controller was smarter, so that it did implement a VLAN-aware
switch, then it wouldn't use FLOOD here.)

> I'm not sure if this is the right function where OUTPUT:FLOOD is processed.

Yes, you found the right function.

> Best regards,
> Min Chen
> 
> On Tue, Mar 13, 2012 at 12:17 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > CC: Min Chen <ustcer.tonychan at gmail.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > Min, here is a little documentation for the function that
> > confused you.
> >
> >  AUTHORS                |    1 +
> >  ofproto/ofproto-dpif.c |   18 +++++++++++++++++-
> >  2 files changed, 18 insertions(+), 1 deletions(-)
> >
> > diff --git a/AUTHORS b/AUTHORS
> > index 958d7f5..bb3cede 100644
> > --- a/AUTHORS
> > +++ b/AUTHORS
> > @@ -114,6 +114,7 @@ Michael Hu              mhu at nicira.com
> >  Michael Mao             mmao at nicira.com
> >  Mike Bursell            mike.bursell at citrix.com
> >  Mike Kruze              mkruze at nicira.com
> > +Min Chen                ustcer.tonychan at gmail.com
> >  Murphy McCauley         murphy.mccauley at gmail.com
> >  Mikael Doverhag         mdoverhag at nicira.com
> >  Niklas Andersson        nandersson at nicira.com
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 7ad6569..ac14fee 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -6322,6 +6322,13 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int vid)
> >     return hash_2words(realdev_ofp_port, vid);
> >  }
> >
> > +/* Returns the ODP port number of the Linux VLAN device that corresponds
> > to
> > + * 'vlan_tci' on the network device with port number 'realdev_odp_port' in
> > + * 'ofproto'.  For example, given 'realdev_odp_port' of eth0 and
> > 'vlan_tci' 9,
> > + * it would return the port number of eth0.9.
> > + *
> > + * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
> > + * function just returns its 'realdev_odp_port' argument. */
> >  static uint32_t
> >  vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> >                        uint32_t realdev_odp_port, ovs_be16 vlan_tci)
> > @@ -6358,9 +6365,18 @@ vlandev_find(const struct ofproto_dpif *ofproto,
> > uint16_t vlandev_ofp_port)
> >     return NULL;
> >  }
> >
> > +/* Returns the OpenFlow port number of the "real" device underlying the
> > Linux
> > + * VLAN device with OpenFlow port number 'vlandev_ofp_port' and stores the
> > + * VLAN VID of the Linux VLAN device in '*vid'.  For example, given
> > + * 'vlandev_ofp_port' of eth0.9, it would return the OpenFlow port number
> > of
> > + * eth0 and store 9 in '*vid'.
> > + *
> > + * Returns 0 and does not modify '*vid' if 'vlandev_ofp_port' is not a
> > Linux
> > + * VLAN device.  Unless VLAN splinters are enabled, this is what this
> > function
> > + * always does.*/
> >  static uint16_t
> >  vsp_vlandev_to_realdev(const struct ofproto_dpif *ofproto,
> > -                   uint16_t vlandev_ofp_port, int *vid)
> > +                       uint16_t vlandev_ofp_port, int *vid)
> >  {
> >     if (!hmap_is_empty(&ofproto->vlandev_map)) {
> >         const struct vlan_splinter *vsp;
> > --
> > 1.7.2.5
> >
> >