[ovs-dev] [PATCH] ofp-actions: Don't encode variable length fields using NXAST_REG_LOAD.

[Jesse Gross]

Currently, when using an OpenFlow 1.0 connection to encode a
tunnel metadata set field action, a series of NXAST_REG_LOADs
are emitted. The result is something like this:

actions=load:0xa->NXM_NX_TUN_METADATA0[0..63],load:0->
NXM_NX_TUN_METADATA0[64..127],load:0->NXM_NX_TUN_METADATA0[128..191],
load:0->NXM_NX_TUN_METADATA0[192..255],load:0->NXM_NX_TUN_METADATA0
[256..319],load:0->NXM_NX_TUN_METADATA0[320..383],load:0->
NXM_NX_TUN_METADATA0[384..447],load:0->NXM_NX_TUN_METADATA0[448..511],
load:0->NXM_NX_TUN_METADATA0[512..575],load:0->NXM_NX_TUN_METADATA0
[576..639],load:0->NXM_NX_TUN_METADATA0[640..703],load:0->
NXM_NX_TUN_METADATA0[704..767],load:0->NXM_NX_TUN_METADATA0[768..831],
load:0->NXM_NX_TUN_METADATA0[832..895],load:0->NXM_NX_TUN_METADATA0
[896..959],load:0->NXM_NX_TUN_METADATA0[960..991]

This happens because tunnel metadata is seen as a maximum size field
and so many loads need to be emitted to cover the entire thing. Besides
being ugly (this shows up when using ovs-ofctl in the default
configuration), it exposes the internal size of the field. While this
shouldn't be an issue since specific protocol fields (such as Geneve
options) have fixed max sizes even if the OVS implementation is extended,
it's still not a great idea.

If we instead use NXAST_REG_LOAD2 in cases where there isn't a suitable
OpenFlow alternative, both problems are avoided:

actions=set_field:0xa->tun_metadata0

This prefers NXAST_REG_LOAD2 for variable length fields since they would
all generally have the same problems. In addition, since the concept of
this type of field is fairly new, there are no backwards compatibility
issues.

Signed-off-by: Jesse Gross <jesse at nicira.com>
---
 lib/ofp-actions.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index ad88c6e..582c22c 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -2271,11 +2271,11 @@ static void
 set_field_to_nxast(const struct ofpact_set_field *sf, struct ofpbuf *openflow)
 {
     /* If 'sf' cannot be encoded as NXAST_REG_LOAD because it requires an
-     * experimenter OXM (or if it came in as NXAST_REG_LOAD2), encode as
-     * NXAST_REG_LOAD2.  Otherwise use NXAST_REG_LOAD, which is backward
-     * compatible. */
+     * experimenter OXM or is variable length (or if it came in as
+     * NXAST_REG_LOAD2), encode as NXAST_REG_LOAD2.  Otherwise use
+     * NXAST_REG_LOAD, which is backward compatible. */
     if (sf->ofpact.raw == NXAST_RAW_REG_LOAD2
-        || !mf_nxm_header(sf->field->id)) {
+        || !mf_nxm_header(sf->field->id) || sf->field->variable_len) {
         struct nx_action_reg_load2 *narl OVS_UNUSED;
         size_t start_ofs = openflow->size;
 
-- 
2.1.4