[ovs-dev] [PATCH 1/4] classifier, flow : Set CFI as part of VID

Simon Horman horms at verge.net.au
Thu Jul 5 00:52:37 UTC 2012


On Wed, Jul 04, 2012 at 05:49:30PM -0700, Ben Pfaff wrote:
> On Thu, Jul 05, 2012 at 09:47:29AM +0900, Simon Horman wrote:
> > On Wed, Jul 04, 2012 at 05:34:43PM -0700, Ben Pfaff wrote:
> > > On Thu, Jul 05, 2012 at 09:27:05AM +0900, Simon Horman wrote:
> > > > On Wed, Jul 04, 2012 at 02:55:52PM -0700, Ben Pfaff wrote:
> > > > > On Wed, Jul 04, 2012 at 05:50:41PM +0900, Simon Horman wrote:
> > > > > > PCP depends on the presence of VID so it seems to make sense
> > > > > > to set the CFI bit as part of setting the VID rather than the PCP.
> > > > > > 
> > > > > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > > > > 
> > > > > The following change definitely can't be correct because OFP_VLAN_NONE
> > > > > is 0xffff:
> > > > > >  void
> > > > > >  flow_set_vlan_vid(struct flow *flow, ovs_be16 vid)
> > > > > >  {
> > > > > > +    vid &= htons(VLAN_VID_MASK | VLAN_CFI);
> > > > > >      if (vid == htons(OFP_VLAN_NONE)) {
> > > > > >          flow->vlan_tci = htons(0);
> > > > > >      } else {
> > > > > > -        vid &= htons(VLAN_VID_MASK);
> > > > > >          flow->vlan_tci &= ~htons(VLAN_VID_MASK);
> > > > > >          flow->vlan_tci |= htons(VLAN_CFI) | vid;
> > > > > >      }
> > > > > 
> > > > > (If the unit tests don't catch that then we need to improve the unit
> > > > > tests.)
> > > > > 
> > > > > Stepping back, I think that there might be some missing context here.
> > > > > In particular, these functions that you're looking at date back one way
> > > > > or another to the earliest days of Open vSwitch, where OpenFlow
> > > > > (pre-)1.0 was the only standard and NXM hadn't been thought of yet.  So
> > > > > they implicitly work with what OF1.0 expects, such as OFP_VLAN_NONE.
> > > > > Perhaps that needs to go in a comment or in the function name
> > > > > (e.g. "flow_set_vlan_vid_of10").
> > > > 
> > > > Yes, I think it would be a good idea for me to take a step back.
> > > > 
> > > > I will see about making flow_set_vlan_vid_of10 and flow_set_vlan_vid_of12
> > > > functions and see if that works out a bit better.
> > > 
> > > That sounds like a good plan to me.
> > 
> > Actually, I am still somewhat confused.
> > 
> > It seems to me that flow_set_vlan_vid() is only used in conjunction
> > with the MFF_VLAN_VID field id. But it is unclear to me how that may
> > be used other than with OXM. Likewise with MFF_VLAN_PCP.
> > 
> > Am I missing something?
> 
> I'm pretty sure it's what ends up getting called in e.g. ovs-ofctl if
> you do something like "ovs-ofctl add-flow br0 dl_vlan=123,actions=drop".
> (But I didn't check, just now, to be sure.)

Thanks, I will check.



More information about the dev mailing list