On 11/16/13 1:21 AM, Ben Pfaff wrote:
On Tue, Nov 12, 2013 at 04:36:25PM +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 (notable
flow_extract()), in order to differentiate between layer 2 and layer 3
packets. struct flow has now a new 'noeth' 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.
Signed-off-by: Lorand Jakab <loja...@cisco.com>
This needs a rebase due to changes in dpif-netdev.
Will do. I always keep a rebased version on github:
https://github.com/ljakab/openvswitch/commits/l3_v2
BTW, my patch had the exact same changes that causes the need for rebase.
I get build assertion failures in flow.h. I build on 32-bit x86,
maybe you're using 64-bit:
../lib/flow.h:127:1: error: bit-field 'build_assert_failed' has negative width
(-1)
BUILD_ASSERT_DECL(offsetof(struct flow, nw_frag) + 1
^
It's probably a bad idea to put noeth at the very top of struct flow,
because it means that it is guaranteed to be followed by 3 or 7 bytes
(on 32 or 64 bit archs) of padding.
Yes, I build on 64-bit x86. Will move the new member somewhere else,
and test builds on 32-bit x86 as well.
I'm not a fan of "negative" variable names, like "noeth", because it
makes logic harder to understand than necessary. Something like a
"base_layer" member that could be set to 2 or 3, or perhaps an enum
typed member, might be better.
Agree, poor naming on my part, but had no better ideas. This is exactly
the kind of feedback I was looking for.
The code doesn't entirely conform to OVS conventions, e.g. OVS always
puts {} around conditional statements.
Sorry, will fix.
Functions comments (e.g. flow_extract()) need updates to explain the
new conventions.
Ack.
struct flow could use an expansion of its top-level comment to explain
the new conventions also.
Ack.
The statement
+ packet->l2_5 = NULL;
is now in two places unnecessarily.
Will remove redundant occurrences.
I don't think there's a good reason for the new 'goto' in
flow_extract(). I think an 'else' would work just as well.
OK.
Have you thought about interactions with OpenFlow yet? As-is, there
are going to be some oddities.
I was focusing more on getting it to work first :) I assume you're
right about the oddities, and I was thinking something along the lines
of reviewing all OF actions and see if they make sense in an L3 context
or not. If you (or anyone else) has any ideas on what else I should
look at, what else might fail/misbehave, please let me know.
For example, the flow
"dl_dst=00:00:00:00:00:00, actions=drop" will now drop all L3 packets,
I believe. Presumably we do not want that. I guess any flow that
matches on dl_dst or dl_src should implicitly also match noeth=false.
I missed this, thanks for pointing it out. And there are probably some
other cases when noeth=false should be needed. Maybe make it (well
"base_layer") a mandatory match, like in_port, metadata, etc. ?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
