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 <et...@nicira.com>
> Signed-off-by: Ethan Jackson <et...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
