[ovs-dev] [xc_v2 4/4] ofproto-dpif: Track relevant fields for wildcarding and add xout cache.

Justin Pettit jpettit at nicira.com
Tue Jun 11 07:15:02 UTC 2013


On Jun 7, 2013, at 2:51 AM, "Rajahalme, Jarno (NSN - FI/Espoo)" <jarno.rajahalme at nsn.com> wrote:

>> @@ -3764,7 +3880,7 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
>>    struct ofpbuf *packet;
>> 
>>    subfacet = subfacet_create(facet, miss, now);
>> -    want_path = subfacet->facet->xout.slow ? SF_SLOW_PATH : SF_FAST_PATH;
>> +    want_path = subfacet->facet->xc->xout.slow ? SF_SLOW_PATH : SF_FAST_PATH;
>> 
> 
> facet->xc->xout.slow works as well.

That was in the existing code, but I agree it's shorter, so I changed it.

>> @@ -5013,8 +5149,10 @@ facet_revalidate(struct facet *facet)
>>    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
>>    struct rule_dpif *new_rule;
>>    struct subfacet *subfacet;
>> -    struct xlate_out xout;
>> +    struct flow_wildcards wc;
>> +    struct xout_cache *xc;
>>    struct xlate_in xin;
>> +    bool refresh_subfacets = false;
>> 
>>    COVERAGE_INC(facet_revalidate);
>> 
>> @@ -5037,63 +5175,58 @@ facet_revalidate(struct facet *facet)
>>        }
>>    }
>> 
>> -    new_rule = rule_dpif_lookup(ofproto, &facet->flow);
>> +    flow_wildcards_init_catchall(&wc);
>> +    new_rule = rule_dpif_lookup(ofproto, &facet->flow, &wc);
>> 
>> -    /* Calculate new datapath actions.
>> -     *
>> -     * We do not modify any 'facet' state yet, because we might need to, e.g.,
>> -     * emit a NetFlow expiration and, if so, we need to have the old state
>> -     * around to properly compose it. */
>> -    xlate_in_init(&xin, ofproto, &facet->flow, &facet->initial_vals, new_rule,
>> -                  0, NULL);
>> -    xlate_actions(&xin, &xout);
>> +    xc = lookup_xc(ofproto, &facet->flow);
>> +    if (!xc) {
>> +        ovs_assert(!facet->xc);
>> +        xlate_in_init(&xin, ofproto, &facet->flow, &facet->initial_vals,
>> +                      new_rule, 0, NULL);
>> +        xc = create_xc(&wc, &facet->initial_vals, facet);
>> +        xlate_actions(&xin, &xc->xout);
>> +        insert_xc(ofproto, xc, &facet->flow);
>> +        refresh_subfacets = true;
>> +    } else if (!facet->xc) {
>> +        add_facet_to_xc(xc, facet);
>> +        refresh_subfacets = true;
>> +    }
> 
> refresh_subfacets = true in both cases?

The whole facet_revalidate() handling was getting nasty, so I'm dropping this patch.  Ethan had a better way to handle this caching, which I'll send out as a new patch series soon.

>> /* Translates the 'ofpacts_len' bytes of "struct ofpacts" starting at 'ofpacts'
>> - * into datapath actions in 'odp_actions', using 'ctx'. */
>> + * into datapath actions in 'odp_actions', using 'ctx'.
>> + *
>> + * The caller is responsible for initializing 'xout->wc'.  It will
>> + * likely either be the wildcards from a call to rule_dpif_lookup() or
>> + * flow_wildcards_init_catchall() if no rule was used.
>> + */
>> static void
>> xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>> {
>> @@ -6917,6 +7169,36 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>>    memset(&ctx.base_flow.tunnel, 0, sizeof ctx.base_flow.tunnel);
>>    ctx.orig_tunnel_ip_dst = ctx.xin->flow.tunnel.ip_dst;
>> 
>> +    memset(&ctx.xout->wc.masks.in_port, 0xff,
>> +           sizeof ctx.xout->wc.masks.in_port);
>> +
>> +    if (tnl_port_should_receive(&ctx.xin->flow)) {
>> +        memset(&ctx.xout->wc.masks.tunnel, 0xff,
>> +               sizeof ctx.xout->wc.masks.tunnel);
>> +    }
>> +
>> +    /* Disable most wildcarding for NetFlow. */
>> +    if (xin->ofproto->netflow) {
>> +        memset(&ctx.xout->wc.masks.dl_src, 0xff,
>> +               sizeof ctx.xout->wc.masks.dl_src);
>> +        memset(&ctx.xout->wc.masks.dl_dst, 0xff,
>> +               sizeof ctx.xout->wc.masks.dl_dst);
>> +        memset(&ctx.xout->wc.masks.dl_type, 0xff,
>> +               sizeof ctx.xout->wc.masks.dl_type);
>> +        memset(&ctx.xout->wc.masks.vlan_tci, 0xff,
>> +               sizeof ctx.xout->wc.masks.vlan_tci);
>> +        memset(&ctx.xout->wc.masks.nw_proto, 0xff,
>> +               sizeof ctx.xout->wc.masks.nw_proto);
>> +        memset(&ctx.xout->wc.masks.nw_src, 0xff,
>> +               sizeof ctx.xout->wc.masks.nw_src);
>> +        memset(&ctx.xout->wc.masks.nw_dst, 0xff,
>> +               sizeof ctx.xout->wc.masks.nw_dst);
>> +        memset(&ctx.xout->wc.masks.tp_src, 0xff,
>> +               sizeof ctx.xout->wc.masks.tp_src);
>> +        memset(&ctx.xout->wc.masks.tp_dst, 0xff,
>> +               sizeof ctx.xout->wc.masks.tp_dst);
>> +    }
>> +
> 
> Use the new flow_mask_hash_fields(&ctx.xout->wc, NX_HASH_FIELDS_SYMMETRIC_L4) for this?
> 
> Also, could define new NX_HASH_FIELDS_* values for MPLS, NW_PROTO, etc. and use to set the masks above. 

I agree the current method is a bit too verbose, but using that function seems confusing since it's used for a very different purpose, so I think I'd rather leave it as-is for now.

--Justin





More information about the dev mailing list