From 4fe04a7e6ded9ec24b7379f6227d15d3ff4f577f Mon Sep 17 00:00:00 2001 From: Jesse Gross Date: Thu, 27 Aug 2015 18:40:45 -0700 Subject: [PATCH] ofp-actions: Don't encode variable length fields using NXAST_REG_LOAD. 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 Acked-by: Ben Pfaff --- 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 ad88c6eb51b..582c22c78eb 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;