[ovs-dev] [PATCH 2/4] ofproto-dpif: Store the initial tunnel IP TOS values for later use.

[Justin Pettit]

On Feb 14, 2013, at 12:04 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Feb 13, 2013 at 03:31:43PM -0800, Justin Pettit wrote:
>> When a packet arrives on an IP tunnel, store the TOS value for later
>> use.  This value will be used in a couple of future commits.
>> 
>> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> 
> ...
> 
>> @@ -4922,6 +4928,7 @@ flow_push_stats(struct rule_dpif *rule,
>>     ofproto_rule_update_used(&rule->up, stats->used);
>> 
>>     initial_vals.vlan_tci = flow->vlan_tci;
>> +    initial_vals.tunnel_ip_tos = 0;
>>     action_xlate_ctx_init(&ctx, ofproto, flow, &initial_vals, rule,
>>                           0, NULL);
>>     ctx.resubmit_stats = stats;
> 
> I think that ideally we should obtain the initial_vals from a subfacet
> here.  Otherwise I think we could end up doing a translation here that
> is different from the initial one.  For example, for the initial
> translation we might drop the packet due to the ECN bits, and we don't
> want to do something different on facet_push_stats().
> 
> Same comment in facet_learn() except that practically speaking I don't
> think it can make a difference there.
> 
> I'm not sure that initial_vals could be different on different
> subfacets.  Maybe we should move it to the facet.

As I discussed off-line, there may have also been an issue the TCI, too.  I re-architected things a bit in my new revision so that both should be handled.

>> @@ -6612,6 +6621,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
>>         uint32_t local_odp_port;
>> 
>>         initial_vals.vlan_tci = ctx->base_flow.vlan_tci;
>> +        initial_vals.tunnel_ip_tos = 0;
> 
> I think that the initializer here should come from
> ctx->base_flow.tunnel.ip_tos since that's what we initialized from
> initial_vals->tunnel_ip_tos back in action_xlate_ctx_init().

Good point.

I'll send out my revised version shortly.

--Justin