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

Ben Pfaff blp at nicira.com
Fri Jun 7 20:04:42 UTC 2013


On Fri, Jun 07, 2013 at 02:03:40AM -0700, Justin Pettit wrote:
> Dynamically determines the flow fields that were relevant in
> processing flows based on the OpenFlow flow table and switch
> configuration.  The immediate use for this functionality is to
> generate an action translation cache for similar flows.  This
> yields a roughly 40% improvement in flow set up rates for a
> complicated flow table.  (Another 40% improvement should be
> trivially achievable by more efficiently deleting facets.)
> 
> More importantly, these wildcards will be used to determine what to
> wildcard for the forthcoming megaflow patches that will allow
> wildcarding in the kernel, which will provide significant flow
> set up improvements.
> 
> The approach to tracking fields and the idea of caching action
> translations was based on an impressive prototype by Ethan Jackson.
> 
> Co-authored-by: Ethan Jackson <ethan at nicira.com>
> Signed-off-by: Ethan Jackson <ethan at nicira.com>

I think that nxm_execute_reg_move() and nxm_execute_stack_push() could
just mask out the subfield that is read, not the whole field that
contains the subfield.

The 'wc' member of xlate_out is weird.  It is not purely an output of
xlate, it is actually an in/out, and xlate_actions() depends on the
caller to have put something sensible in it.  Well, I guess if you
start with garbage there and don't care about what you end up with,
then it's garbage-in garbage-out, no harm done, but it means that you
have to carefully check xlate_actions() users to see whether they
initialize it and, if they don't, then whether that matters.  When I
do that, I see a problem in handle_flow_miss_without_facet(): it looks
to me that the wc mask created by xlate_actions() is uninitialized.

There's a typo "xalate_actions()" in a comment in
handle_flow_miss_without_facet().

In handle_flow_miss_without_facet(), if you move the declaration of
'skip_stats' inside the LIST_FOR_EACH, then you can change:
        if (xc) {
            /* Don't update a new xc's stats, since they were captured
             * during the xalate_actions(). */
            if (!skip_stats) {
                dpif_flow_stats_extract(&miss->flow, packet, now, &xc->stats);
                xc->stats.n_packets++;
                xc->stats.n_bytes += stats.n_bytes;
                xc->stats.tcp_flags |= stats.tcp_flags;
                xc->stats.used = MAX(xc->stats.used, stats.used);
            } else {
                skip_stats = false;
            }
        }
to just:
        if (xc && !skip_stats) {
            /* Don't update a new xc's stats, since they were captured
             * during the xlate_actions(). */
            dpif_flow_stats_extract(&miss->flow, packet, now, &xc->stats);
            xc->stats.n_packets++;
            xc->stats.n_bytes += stats.n_bytes;
            xc->stats.tcp_flags |= stats.tcp_flags;
            xc->stats.used = MAX(xc->stats.used, stats.used);
        }

In facet_create(), the declaration of 'xin' can be moved to an inner
block.

I think that facet_lookup_valid() can leave facets with null
'facet->xc'.  Suppose, for example, that need_revalidate is true.
Then facet_lookup_valid() will delete all xcs, but it will only
facet_revalidate() a single facet.

I skipped over facet_revalidate() because I think you mentioned
off-list that it is not yet ready for review.

This code in destroy_xc():
        facet->packet_count += xc->stats.n_packets;
        facet->byte_count += xc->stats.n_bytes;
        facet->used = MAX(facet->used, xc->stats.used);
        facet->tcp_flags |= xc->stats.tcp_flags;
is similar to this code in subfacet_update_stats():
        facet->used = MAX(facet->used, stats->used);
        facet->packet_count += stats->n_packets;
        facet->byte_count += stats->n_bytes;
        facet->tcp_flags |= stats->tcp_flags;
so perhaps they can be written as a single helper.

I just noticed this code in add_mirror_actions().  It is really
expensive and does not make sense to me.  bitmap_scan() will return
4096 only if no bits are set in m->vlans, but that is not a useful
mirroring configuration (such a mirror will never mirror anything,
because no VLANs are selected):
        if (m->vlans && bitmap_scan(m->vlans, 0, 4096) < 4096) {
            ctx->xout->wc.masks.vlan_tci |= htons(VLAN_CFI | VLAN_VID_MASK);
        }
I think that, instead, I would just write "if (m->vlans) {", because
that would ordinarily mean that some VLANs are selected and others are
not.

Thanks,

Ben.



More information about the dev mailing list