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 <loja...@cisco.com>
Please update the comment on ofp_packet_to_string() to explain the
is_layer3 parameter.
OK.
In parse_odp_packet() it should be unnecessary to explicitly set l2 or
l3 to NULL. ofpbuf_use_stub() does that for us.
Removed.
This deletes the description of setting l3 from the comment on
flow_extract(), but flow_extract() still sets l3 (for L2 packets).
I restored and modified the comment appropriately.
In flow_extract() there's no minimum size check in the L3 branch. At
a minimum we need one byte to distinguish v4/v6, or we could check for
at least IP_HEADER_LEN.
I added a check for at least IP_HEADER_LEN.
In flow_extract() it seems odd to set l2 to NULL just after we
verified that it is NULL.
I removed it.
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.
In meta-flow.c, do the VLAN fields need an MFP_ETHERNET prerequisite?
I think so, and MPLS fields as well in the future.
Lots of unneeded () in compose_output_action__():
+ if ((in_xport) && !(in_xport->is_layer3) && xport->is_layer3) {
+ odp_put_pop_eth_action(&ctx->xout->odp_actions);
+ }
+
+ if (flow->base_layer == LAYER_3 && !(xport->is_layer3)) {
Removed extra ().
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.
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?
-Lori
I think that we will need to include an indication of the packet type
(L2 or IPv4 or IPv6, I guess) in flow_metadata, at least eventually,
for proper OpenFlow support.
ofp_print_packet_in() and ofp_print_packet_out() will eventually need
to figure out whether a packet is L2 or L3 by looking at the flow,
although this requires additional OpenFlow extensions.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
