[ovs-dev] [PATCH v2 2/3] userspace: add layer 3 flow and switching support

Ben Pfaff blp at nicira.com
Wed Jan 8 20:22:08 UTC 2014


On Wed, Jan 08, 2014 at 07:06:32PM +0200, Lori Jakab wrote:
> On 12/30/13 9:28 PM, Ben Pfaff wrote:
> >On Tue, Dec 24, 2013 at 04:02:35PM +0200, Lorand Jakab wrote:
> >>This commit relaxes the assumption that all packets have an Ethernet
> >>header, and adds support for layer 3 flows.  For each packet received on
> >>the Linux kernel datapath the l2 and l3 members of struct ofpbuf are
> >>intialized appropriately, and some functions now expect this (notably
> >>flow_extract()), in order to differentiate between layer 2 and layer 3
> >>packets.  struct flow has now a new 'base_layer' member, because we
> >>cannot assume that a flow has no Ethernet header when eth_src and
> >>eth_dst are 0.  For layer 3 packets, the protocol type is still stored
> >>in the eth_type member.
> >>
> >>Switching L2->L3 and L3->L2 are both implemented by adding the pop_eth
> >>and push_eth actions respectively when a transition is detected.  The
> >>push_eth action puts 0s on both source and destination MACs.  These
> >>addresses can be modified with mod_dl_dst and mod_dl_src actions.
> >>
> >>Added new prerequisite MFP_ETHERNET for fields MFF_ETH_SRC and
> >>MFF_ETH_DST to avoid detecting layer 2 flows with all-zero MAC addresses
> >>as layer 3.
> >>
> >>Signed-off-by: Lorand Jakab <lojakab at cisco.com>

...

> >In struct flow, I'm not sure that it's safe to both use an enum and
> >assert on the size of the structure, because the compiler is allowed
> >to choose any appropriate size for an enum.  I believe that the System
> >V ABI says that an enum should have type 'int', but I doubt that this
> >is universal across all ABIs, so I'd be inclined to use some explicit
> >type for the member, even though we use the LAYER_* values.  (Another
> >idea would be to just use '2' or '3' as the values and skip the
> >enemas.)
> 
> I changed it to uint32_t.  I would like to keep the enum, because a
> match initialized to all zeroes will be automatically LAYER_2 and
> there is no need to update the code for explicitly setting it.

OK, that's a good point, thanks.

> >In meta-flow.c, do the VLAN fields need an MFP_ETHERNET prerequisite?
> 
> I think so, and MPLS fields as well in the future.

For what it's worth, I think I was pointing out here that the VLAN
fields lacked that prerequisite, not really asking a question.  I
agree with your answer, of course.

> >I find myself wondering whether we should require OpenFlow to
> >explicitly push and pop Ethernet headers for output, instead of
> >automatically pushing and popping.  There could be some pretty weird
> >results if nothing sets an Ethernet source or dest address and outputs
> >all-zeros addresses to an L2 port as a result.  And it would also be
> >pretty weird stripping the Ethernet header off, say, an ARP packet and
> >outputting it to an L3 port.
> 
> It's definitely less error prone if we do that, I agree.  However,
> the NORMAL action would not send flooded packets to L3 ports, and
> there is no MAC learning for packets received on L3 ports.  There
> needs to be a rule to send traffic explicitly to a L3 port.  If that
> rule includes packets that shouldn't go there, it's the user's job
> to fix it.  Same for receiving.  Received packets should have
> explicit rules to set the destination MAC and output port.  If no
> such rule exists, they should be dropped.  To the best of my
> knowledge, that's what happens currently.
> 
> I don't have a strong opinion either way, or of both operating modes
> should be supported, so I'll do as you prefer.  I think I slightly
> favor your approach, since then we don't have to eventually
> implement ARP lookup internally, and it's the user/controller's job
> to set MAC addresses.

OK.  Thanks for humoring me, and for the analysis.  I had not thought
about NORMAL or flooding or MAC learning before.

> >OpenFlow
> >--------
> >
> >There is an OpenFlow extensibility working group proposal that matches
> >the requirements here:
> >         https://rs.opennetworking.org/bugs/browse/EXT-112
> >I suggest implementing OpenFlow support as described there.
> 
> Is this required before the series goes in?

If there is going to be OpenFlow support, I want it considered,
otherwise it is not necessary yet.

Thanks,

Ben.



More information about the dev mailing list