On Apr 27, 2013, at 0:43 , ext Jesse Gross wrote:
> On Thu, Apr 18, 2013 at 8:07 AM, Jarno Rajahalme
> <jarno.rajaha...@nsn.com> wrote:
>> diff --git a/lib/match.c b/lib/match.c
>> index 2aa4e89..222e5d7 100644
>> --- a/lib/match.c
>> +++ b/lib/match.c
>> @@ -133,13 +133,9 @@ match_wc_init(struct match *match, const struct flow
>> *flow)
>> void
>> match_init_exact(struct match *match, const struct flow *flow)
>> {
>> - ovs_be64 tun_id = flow->tunnel.tun_id;
>> -
>> match->flow = *flow;
>> match->flow.skb_priority = 0;
>> match->flow.skb_mark = 0;
>> - memset(&match->flow.tunnel, 0, sizeof match->flow.tunnel);
>> - match->flow.tunnel.tun_id = tun_id;
>
> Since we're not allowing matching on these fields yet, it seems like
> semantically it makes more sense to include this change as part of the
> last patch. In this case, it probably doesn't matter much in practice
> though.
You are right. I'll move it this.
>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 33b09c6..b88a2d4 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -6849,23 +6847,23 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
>> + * - Tunnel metadata as received is retained in both 'flow' and
>> + * 'base_flow'. This will prevent packet output with the exact same
>> + * headers as it was received with (which would not make sense since
>> + * the destination would be us). In effect, this emulates the kernel
>> + * behavior of clearing the tunnel metadata before action processing.
>
> This makes me nervous because it desynchronizes userspace's view from
> the kernel's. Historically, this has caused problems (the ones that
> Ben cited) when we try to generate actions based on what we think has
> changed. In this case, the effect is likely fairly limited - you won't
> be able to send a packet with the exact same parameters that it came
> in on but it doesn't really seem like a good direction.
In retrospect it would be cleaner to zero out tunnel metadata in 'base_flow'
(as before) and keep it only in 'flow' to allow later matching on it.
This way 'base_flow' would reflect how kernel sees tunnel metadata at the
start of action processing (none). The fact that 'flow' is different from
'base_flow' in this regard does not matter, as tunnel metadata is only ever
committed to kernel with tunnel output, and tnl_port_send() will take care
of setting the flow.tunnel as need be.
Another thing that I came to think only when reading Ben's new tutorial:
Output to the input port is skipped. This would be a problem if you only
have one generic flow based GRE port (as enabled by the last patch in
this series), and you should forward GRE input to another GRE output.
This could be fixed by always allowing output (in xlate_output_action())
to a tunnel that has cfg.ip_dst_flow set, even if the output port is the
same as the input port. Thoughts?
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
