[ovs-dev] [packet_in 13/13] openflow: New Nicira Extended PACKET_IN format.

Ethan Jackson ethan at nicira.com
Tue Jan 3 21:46:47 UTC 2012


>    ../lib/ofp-util.c: In function 'ofputil_decode_packet_in':
>    ../lib/ofp-util.c:1613: error: too many arguments to function 'nx_pull_match'
>    ../lib/ofp-util.c: In function 'ofputil_encode_packet_in':
>    ../lib/ofp-util.c:1693: error: too many arguments to function 'nx_put_match'

This was due to some conflicting changes.  The second version of the
patch I sent out should have fixed it.  The fix is trivial,  I'm about
to resend the patch anyways so you can try the newest version in a
bit.

> There's a double blank line above the definition of
> NXT_SET_PACKET_IN_FORMAT.  (Did you intend to put some high-level
> comment there?)  Also a blank line after the new NXT_PACKET_IN.

I didn't intend to, but it's not a bad idea so I've added one.

> It might be reasonable to reuse the NXFF_* values for the 'format'
> member in nxt_set_packet_in_format, instead of defining new NXPIF_*
> values.  Or you could even define a new NXFF_NXM2 that is NXFF_NXM
> plus packet-in support, which would allow you to drop the separate
> NXT_SET_PACKET_IN_FORMAT message.  But the way you have it is also
> fine, so don't feel obligated to change anything.

I find this all a bit confusing.  I was under the impression that the
FF in NXFF stands for Flow Format.  This kind of gets at what I'm
changing, but not perfectly.  I suppose using the NXFF values works
until there is no longer a one to one mapping between the flow format
and the packet_in formats.  It doesn't seem impossible to me that
something like this could happen which is why I implemented it this
way.  At any rate, I don't feel strongly about it either way.  I think
the current definition most clear gets at what the feature's intention
is, so I'm slightly inclined to leave it, but I could change it as
well.

> I'd extend the comment on struct nxt_packet_in to mention a few more
> points:
>
>        * This format is a slightly extended version of the OpenFlow
>          1.2 packet-in (with the cookie added).
>
>        * The meaning of the nxm_fields (are we including the whole
>          match or just the metadata? can the controller rely on that
>          in the future? etc.)
>
>        * Whereas in most cases a controller can expect to only get
>          back NXM fields that it set up itself (e.g. flow dumps will
>          ordinarily report only NXM fields from flows that the
>          controller added), packet-ins might contain fields that the
>          controller does not understand, because the switch might
>          support fields (new registers, new protocols, etc.) that the
>          controller does not.  The controller must prepared to
>          tolerate these.
>
> What's the meaning/value of table_id and cookie in struct
> nxt_packet_in if the reason is OFPR_MISS?

I changed the nxt_packet_in comment to the following:

/* NXT_PACKET_IN (analogous to OFPT_PACKET_IN).
 *
 * The NXT_PACKET_IN format is intended to model the OpenFLow-1.2 PACKET_IN
 * with some minor tweaks.  Most notably NXT_PACKET_IN includes the cookie of
 * the rule which triggered the NXT_PACKET_IN message, and the match fields are
 * in NXM format.
 *
 * The match fields in the NXT_PACKET_IN are intended to contain flow
 * processing metadata collected at the time the NXT_PACKET_IN message was
 * triggered.  It is minimally required to contain the NXM_OF_IN_PORT of the
 * packet, but may include other NXM headers such as flow registers.  The match
 * fields are allowed to contain non-metadata (e.g. NXM_OF_ETH_SRC etc).
 * However, this information can typically be found in the packet directly, so
 * it may be redundant.
 *
 * Whereas in most cases a controller can expect to only get back NXM fields
 * that it set up itself (e.g. flow dumps will ordinarily report only NXM
 * fields from flows that the controller added), NXT_PACKET_IN messages might
 * contain fields that the controller does not understand, because the switch
 * might support fields (new registers, new protocols, etc.) that the
 * controller does not.  The controller must prepared to tolerate these.
 *
 * The 'cookie' and 'table_id' fields have no meaning when 'reason' is
 * OFPR_NO_MATCH.  In this case they should be set to 0. */

> tun_id is also a metadata field but this patch doesn't export it via
> packet_in, why not?

This was an oversight.  I'll change it.

> Did you consider adding a struct to encapsulate metadata, e.g. "struct
> flow_metadata" or similar?

I hadn't considered it.  I've tried it just now though and I don't
think it works well.  It's conceptually much simpler, but it's fairly
awkward to fit a flow_metadata structure containing the registers, and
flow_metadata into the struct flow because of our reliance on the
FLOW_SIG_SIZE.  It is hard to get the padding right.  Of course, there
are ways around this which we could utilize in the future.  I'm not
sure it's worth it at the moment though.  Thoughts?

> I don't see why the change to nx-match.h is necessary.

Oops, this is leftover from a previous version of the patch.

 > In ofputil_decode_packet_in(), I think we need a more relaxed version
> of nx_pull_match() that doesn't report an error just because of NXM
> fields that are not understood.

I added a new "bool strict" parameter to nx_pull_match() which ignores
unknown types when false.  I'll send that patch out when I resend the
series.

Ethan



More information about the dev mailing list