On Jun 7, 2013, at 2:51 AM, "Rajahalme, Jarno (NSN - FI/Espoo)"
<[email protected]> 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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
