Thanks, applied to master.
On Mon, Dec 30, 2013 at 03:12:20PM -0800, Romain Lenglet wrote: > This looks good to me. Thanks! > -- > Romain Lenglet > > ----- Original Message ----- > > From: "Ben Pfaff" <[email protected]> > > To: [email protected] > > Cc: "Ben Pfaff" <[email protected]>, "Romain Lenglet" <[email protected]> > > Sent: Monday, December 30, 2013 2:50:59 PM > > Subject: [PATCH v2] ofproto-dpif: Enable NXAST_SAMPLE only if the datapath > > supports it. > > > > This prevents using an older datapath from breaking forwarding. > > > > CC: Romain Lenglet <[email protected]> > > Signed-off-by: Ben Pfaff <[email protected]> > > --- > > v1->v2: Check for variable-length data support in the correct place > > (see Romain's comments in > > https://urldefense.proofpoint.com/v1/url?u=http://patchwork.openvswitch.org/patch/1627/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=zXHlW45sT8%2FAsm%2BMbdJ6DyHSNzIfOMAfy9flPhaH%2FAg%3D%0A&m=AocvK29H8OSuhXPkW%2BHNDFt6Oq4fBfLiLK5ohOjZJ7M%3D%0A&s=31fde9854655304ed23c9d96c031b0fab4d37355f90cd099f69ddc105a0a1d29). > > > > ofproto/ofproto-dpif-xlate.c | 18 ++++++++- > > ofproto/ofproto-dpif-xlate.h | 2 +- > > ofproto/ofproto-dpif.c | 92 > > +++++++++++++++++++++++++++++++++++++++++- > > 3 files changed, 109 insertions(+), 3 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 848c778..558598c 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -87,6 +87,11 @@ struct xbridge { > > enum ofp_config_flags frag; /* Fragmentation handling. */ > > bool has_in_band; /* Bridge has in band control? */ > > bool forward_bpdu; /* Bridge forwards STP BPDUs? */ > > + > > + /* True if the datapath supports variable-length > > + * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. > > + * False if the datapath supports only 8-byte (or shorter) userdata. */ > > + bool variable_length_userdata; > > }; > > > > struct xbundle { > > @@ -249,7 +254,8 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const > > char *name, > > const struct dpif_sflow *sflow, > > const struct dpif_ipfix *ipfix, > > const struct netflow *netflow, enum ofp_config_flags > > frag, > > - bool forward_bpdu, bool has_in_band) > > + bool forward_bpdu, bool has_in_band, > > + bool variable_length_userdata) > > { > > struct xbridge *xbridge = xbridge_lookup(ofproto); > > > > @@ -301,6 +307,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const > > char *name, > > xbridge->frag = frag; > > xbridge->miss_rule = miss_rule; > > xbridge->no_packet_in_rule = no_packet_in_rule; > > + xbridge->variable_length_userdata = variable_length_userdata; > > } > > > > void > > @@ -2521,6 +2528,15 @@ xlate_sample_action(struct xlate_ctx *ctx, > > * the same percentage. */ > > uint32_t probability = (os->probability << 16) | os->probability; > > > > + if (!ctx->xbridge->variable_length_userdata) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > + > > + VLOG_ERR_RL(&rl, "ignoring NXAST_SAMPLE action because datapath " > > + "lacks support (needs Linux 3.10+ or kernel module from " > > + "OVS 1.11+)"); > > + return; > > + } > > + > > ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow, > > &ctx->xout->odp_actions, > > &ctx->xout->wc, > > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h > > index 68076ca..982f1a4 100644 > > --- a/ofproto/ofproto-dpif-xlate.h > > +++ b/ofproto/ofproto-dpif-xlate.h > > @@ -129,7 +129,7 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char > > *name, > > const struct mbridge *, const struct dpif_sflow *, > > const struct dpif_ipfix *, const struct netflow *, > > enum ofp_config_flags, bool forward_bpdu, > > - bool has_in_band) > > + bool has_in_band, bool variable_length_userdata) > > OVS_REQ_WRLOCK(xlate_rwlock); > > void xlate_remove_ofproto(struct ofproto_dpif *) > > OVS_REQ_WRLOCK(xlate_rwlock); > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 52759b5..e48cc00 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -42,6 +42,7 @@ > > #include "netdev-vport.h" > > #include "netdev.h" > > #include "netlink.h" > > +#include "netlink-socket.h" > > #include "nx-match.h" > > #include "odp-util.h" > > #include "odp-execute.h" > > @@ -251,6 +252,11 @@ struct dpif_backer { > > enum revalidate_reason need_revalidate; /* Revalidate all flows. */ > > > > bool recv_set_enable; /* Enables or disables receiving packets. */ > > + > > + /* True if the datapath supports variable-length > > + * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. > > + * False if the datapath supports only 8-byte (or shorter) userdata. */ > > + bool variable_length_userdata; > > }; > > > > /* All existing ofproto_backer instances, indexed by ofproto->up.type. */ > > @@ -552,7 +558,8 @@ type_run(const char *type) > > ofproto->sflow, ofproto->ipfix, > > ofproto->netflow, ofproto->up.frag_handling, > > ofproto->up.forward_bpdu, > > - connmgr_has_in_band(ofproto->up.connmgr)); > > + connmgr_has_in_band(ofproto->up.connmgr), > > + ofproto->backer->variable_length_userdata); > > > > HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) { > > xlate_bundle_set(ofproto, bundle, bundle->name, > > @@ -774,6 +781,8 @@ struct odp_garbage { > > odp_port_t odp_port; > > }; > > > > +static bool check_variable_length_userdata(struct dpif_backer *backer); > > + > > static int > > open_dpif_backer(const char *type, struct dpif_backer **backerp) > > { > > @@ -872,6 +881,7 @@ open_dpif_backer(const char *type, struct dpif_backer > > **backerp) > > close_dpif_backer(backer); > > return error; > > } > > + backer->variable_length_userdata = > > check_variable_length_userdata(backer); > > > > if (backer->recv_set_enable) { > > udpif_set_threads(backer->udpif, n_handlers, n_revalidators); > > @@ -880,6 +890,86 @@ open_dpif_backer(const char *type, struct dpif_backer > > **backerp) > > return error; > > } > > > > +/* Tests whether 'backer''s datapath supports variable-length > > + * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. We > > need > > + * to disable some features on older datapaths that don't support this > > + * feature. > > + * > > + * Returns false if 'backer' definitely does not support variable-length > > + * userdata, true if it seems to support them or if at least the error we > > get > > + * is ambiguous. */ > > +static bool > > +check_variable_length_userdata(struct dpif_backer *backer) > > +{ > > + struct eth_header *eth; > > + struct ofpbuf actions; > > + struct ofpbuf key; > > + struct ofpbuf packet; > > + size_t start; > > + int error; > > + > > + /* Compose a userspace action that will cause an ERANGE error on older > > + * datapaths that don't support variable-length userdata. > > + * > > + * We really test for using userdata longer than 8 bytes, but older > > + * datapaths accepted these, silently truncating the userdata to 8 > > bytes. > > + * The same older datapaths rejected userdata shorter than 8 bytes, so > > we > > + * test for that instead as a proxy for longer userdata support. */ > > + ofpbuf_init(&actions, 64); > > + start = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_USERSPACE); > > + nl_msg_put_u32(&actions, OVS_USERSPACE_ATTR_PID, > > + dpif_port_get_pid(backer->dpif, ODPP_NONE)); > > + nl_msg_put_unspec_zero(&actions, OVS_USERSPACE_ATTR_USERDATA, 4); > > + nl_msg_end_nested(&actions, start); > > + > > + /* Compose an ODP flow key. The key is arbitrary but it must match the > > + * packet that we compose later. */ > > + ofpbuf_init(&key, 64); > > + nl_msg_put_u32(&key, OVS_KEY_ATTR_IN_PORT, 0); > > + nl_msg_put_unspec_zero(&key, OVS_KEY_ATTR_ETHERNET, > > + sizeof(struct ovs_key_ethernet)); > > + nl_msg_put_be16(&key, OVS_KEY_ATTR_ETHERTYPE, htons(0x1234)); > > + > > + /* Compose a packet that matches the key. */ > > + ofpbuf_init(&packet, ETH_HEADER_LEN); > > + eth = ofpbuf_put_zeros(&packet, ETH_HEADER_LEN); > > + eth->eth_type = htons(0x1234); > > + > > + /* Execute the actions. On older datapaths this fails with -ERANGE, on > > + * newer datapaths it succeeds. */ > > + error = dpif_execute(backer->dpif, key.data, key.size, > > + actions.data, actions.size, &packet, false); > > + > > + ofpbuf_uninit(&packet); > > + ofpbuf_uninit(&key); > > + ofpbuf_uninit(&actions); > > + > > + switch (error) { > > + case 0: > > + /* Variable-length userdata is supported. > > + * > > + * Purge received packets to avoid processing the nonsense packet > > we > > + * sent to userspace, then report success. */ > > + dpif_recv_purge(backer->dpif); > > + return true; > > + > > + case ERANGE: > > + /* Variable-length userdata is not supported. */ > > + VLOG_WARN("%s: datapath does not support variable-length userdata " > > + "feature (needs Linux 3.10+ or kernel module from OVS " > > + "1..11+). The NXAST_SAMPLE action will be ignored.", > > + dpif_name(backer->dpif)); > > + return false; > > + > > + default: > > + /* Something odd happened. We're not sure whether variable-length > > + * userdata is supported. Default to "yes". */ > > + VLOG_WARN("%s: variable-length userdata feature probe failed (%s)", > > + dpif_name(backer->dpif), ovs_strerror(error)); > > + return true; > > + } > > +} > > + > > static int > > construct(struct ofproto *ofproto_) > > { > > -- > > 1.7.10.4 > > > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
