On Tue, Feb 24, 2015 at 03:42:06PM -0800, Jarno Rajahalme wrote:
> xlate_actions() now takes an optional recirclulation context as a
s/recirclulation/recirculation/
> parameter and restores OpenFlow pipeline metadata based on it. The
> recirculation context is returned by the xlate_lookup() and may
> contain an action set to be restored, and further actions to be
> executed upon recirculation.
>
> The pipeline metadata is restored using two new internal actions. The
> UNROLL_METADATA action stores the non-packet data in struct flow.
> This is used to restore metadata right after recirculation, and also
> after returning from a patch port, if the patch port recirculated.
> This is needed as pipeline metadata restored for recirculation in this
> case belongs to a different bridge than the bridge that initially
> received the packet.
>
> The UNROLL_XLATE action stores the translation context data visible to
> OpenFlow controllers via PACKET_IN messages. This includes the
> current table number and the current rule cookie. UNROLL_XLATE
> actions are always inserted right after the UNROLL_METADATA actions,
> but also whenever returning back from recursive translation when
> recirculating.
>
> These changes allow the post-MPLS recirculation to properly continue
> with the pipeline metadata that existed at the time of recirculation.
> The internal table is still consulted for bonds.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>Summary:
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
"Clang" says:
../ofproto/ofproto-dpif-rid.c:129:13: error: cast from 'const char *' to
'const struct ofpact *' increases required alignment from 1 to 2
[-Werror,-Wcast-align]
a = (const struct ofpact *)((const char *)n->ofpacts
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../ofproto/ofproto-dpif-rid.c:151:29: error: cast from 'const struct ofpact
*'
to 'const uint64_t *' (aka 'const unsigned long long *') increases
required alignment from 2 to 4 [-Werror,-Wcast-align]
hash = hash_words64((const uint64_t *)ofpacts,
^~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
../ofproto/ofproto-dpif-xlate.c:243:12: error: cast from 'char *' to 'struct
ofpact_unroll_xlate *' increases required alignment from 1 to 4
[-Werror,-Wcast-align]
return (struct ofpact_unroll_xlate *)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../ofproto/ofproto-dpif-xlate.c:4547:28: error: cast from 'const char *' to
'const struct ofpact *' increases required alignment from 1 to 2
[-Werror,-Wcast-align]
xin->ofpacts = (const struct ofpact *)
^~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
Maybe ALIGNED_CAST?
"sparse" says:
../ofproto/ofproto-dpif-rid.c:38:13: warning: non-ANSI function declaration
of function 'recirc_init'
(It wants you to write (void) not ().)
The new actions aren't quite internal enough: if I run, for example,
"ovs-ofctl add-flow br0 unroll_metadata", ovs-ofctl aborts.
ofproto-dpif-rid.h
------------------
The examples at the beginning of the comment could really use some
elaboration. I know what you mean but anyone new to this won't.
Some of the big comment is phrased in terms of changes. That makes
sense to people who knew what the code was before, but I generally think
it's better to phrase comments in terms of the current design because
then people new to the system don't have to learn the history to
understand the present. That's only a small change I think, e.g.:
* Two new fields, recirc_id and dp_hash, are added to the current flow data
* structure. They are both of type uint32_t. In addition, a new action,
* RECIRC, is added.
becomes something like:
Recirculation uses two uint32_t fields, recirc_id and dp, and a RECIRC
action.
and if you want to give the history too by adding another sentence like
the following, then that's helpful, but in my humble opinion it's better
if the history isn't the emphasis:
(These fields and action were added specifically to support
recirculation.)
Another minor change I'd make later for the same reason is to change:
* Both new flow fields are exposed as OpenFlow fields via Nicira extensions.
into something like:
* The recirc_id and dp_hash fields are exposed as OpenFlow fields via
* Nicira extensions.
However, meta-flow.h says that these fields are for internal use only
and not available to a controller, so we should probably not say either
of those things.
The comment says:
* The value recirc_id is used to distinguish a packet from multiple
* iterations of recirculation. A packet initially received is considered of
"iterations" sounds to me like we really only have a single
kind of recirculation but it can happen multiple times. Maybe
"instances"?
s/peformed/performed/:
* peformed after recirculation are derived from the OpenFlow pipeline and are
ofproto-dpif-rid.c
------------------
This comment in recirc_run() gave me pause. 250 ms "should be" enough?
What happens if someone odd happens; I hope that nothing
e.g. dereferences a wild pointer and explodes?
/* Delete the expired. These have been lingering for at least 250 ms,
* which should be enough for any ongoing recirculations to be
* finished. */
I think that recirc_find__() requires 'mutex', because it calls
cmap_find_protected() and 'mutex' is what keeps the cmap from changing.
I guess that should be annotated for Clang? It appears that to me that
recirc_free_id() should take the mutex.
You might annotate next_id, expiring, and expired for Clang as protected
by mutex.
This line in recirc_metadata_hash() makes me hope we're careful enough
about zeroing holes (if any) in ofpacts:
hash = hash_words64((const uint64_t *)ofpacts,
OFPACT_ALIGN(ofpacts_len) / OFPACT_ALIGNTO, hash);
recirc_alloc_id() creates and initializes a struct flow but it doesn't
use it for anything later.
In recirc_free_id(), s/exiting/existing/ or better yet
s/non-exiting/nonexistent/. Also I think this is more of a "warn"
situation (isn't this something that can be recovered from? it's not a
fatal error as far as I can tell):
VLOG_ERR("Freeing non-exiting recirculation ID: %"PRIu32, id);
I'm not sure why recirc_free_ofproto() logs errors. Can't there still
be recirc_ids if there were bonds or if the ofproto didn't go idle and
free all its recirc ids before exit? Or is the higher-level code
supposed to clean up before it kills the recirc id engine?
I need to continue reading this patch starting from
ofproto-dpif-upcall.c.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
