[ovs-dev] [PATCH v2] ofproto-dpif: Enable NXAST_SAMPLE only if the datapath supports it.

Ben Pfaff blp at nicira.com
Mon Dec 30 23:35:59 UTC 2013


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" <blp at nicira.com>
> > To: dev at openvswitch.org
> > Cc: "Ben Pfaff" <blp at nicira.com>, "Romain Lenglet" <rlenglet at vmware.com>
> > 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 <rlenglet at vmware.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > 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
> > 
> > 



More information about the dev mailing list