[ovs-dev,action,upcall,meters,2/5] ofproto-dpif: Add 'meter_ids' to backer

Submitted by andy zhou on April 14, 2017, 7:46 p.m.

Details

Message ID 1492199182-4479-2-git-send-email-azhou@ovn.org
State Superseded
Headers show

Commit Message

andy zhou April 14, 2017, 7:46 p.m.
Add 'meter_ids', an id-pool object to manage datapath meter id, i.e.
provider_meter_id.

Currently, only userspace datapath supports meter, and it implements
the provider_meter_id management. Moving this function to 'backer'
allows other datapath implementation to share the same logic.

Signed-off-by: Andy Zhou <azhou@ovn.org>
---
 lib/dpif-netdev.c      | 24 ------------------------
 ofproto/ofproto-dpif.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 ofproto/ofproto-dpif.h |  4 ++++
 3 files changed, 46 insertions(+), 26 deletions(-)

Comments

andy zhou April 14, 2017, 7:55 p.m.
Please ignore this patch.

On Fri, Apr 14, 2017 at 12:46 PM, Andy Zhou <azhou@ovn.org> wrote:
> Add 'meter_ids', an id-pool object to manage datapath meter id, i.e.
> provider_meter_id.
>
> Currently, only userspace datapath supports meter, and it implements
> the provider_meter_id management. Moving this function to 'backer'
> allows other datapath implementation to share the same logic.
>
> Signed-off-by: Andy Zhou <azhou@ovn.org>
> ---
>  lib/dpif-netdev.c      | 24 ------------------------
>  ofproto/ofproto-dpif.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  ofproto/ofproto-dpif.h |  4 ++++
>  3 files changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a14a2ebb5b2a..d5417162b7af 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -260,7 +260,6 @@ struct dp_netdev {
>      /* Meters. */
>      struct ovs_mutex meter_locks[N_METER_LOCKS];
>      struct dp_meter *meters[MAX_METERS]; /* Meter bands. */
> -    uint32_t meter_free;                 /* Next free meter. */
>
>      /* Protects access to ofproto-dpif-upcall interface during revalidator
>       * thread synchronization. */
> @@ -3896,9 +3895,6 @@ dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id,
>      struct dp_meter *meter;
>      int i;
>
> -    if (mid == UINT32_MAX) {
> -        mid = dp->meter_free;
> -    }
>      if (mid >= MAX_METERS) {
>          return EFBIG; /* Meter_id out of range. */
>      }
> @@ -3958,21 +3954,6 @@ dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id,
>          dp->meters[mid] = meter;
>          meter_unlock(dp, mid);
>
> -        meter_id->uint32 = mid; /* Store on success. */
> -
> -        /* Find next free meter */
> -        if (dp->meter_free == mid) { /* Now taken. */
> -            do {
> -                if (++mid >= MAX_METERS) { /* Wrap around */
> -                    mid = 0;
> -                }
> -                if (mid == dp->meter_free) { /* Full circle */
> -                    mid = MAX_METERS;
> -                    break;
> -                }
> -            } while (dp->meters[mid]);
> -            dp->meter_free = mid; /* Next free meter or MAX_METERS */
> -        }
>          return 0;
>      }
>      return ENOMEM;
> @@ -4027,11 +4008,6 @@ dpif_netdev_meter_del(struct dpif *dpif,
>          meter_lock(dp, meter_id);
>          dp_delete_meter(dp, meter_id);
>          meter_unlock(dp, meter_id);
> -
> -        /* Keep free meter index as low as possible */
> -        if (meter_id < dp->meter_free) {
> -            dp->meter_free = meter_id;
> -        }
>      }
>      return error;
>  }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6a5ffb94fa94..a026d4913731 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -662,6 +662,7 @@ close_dpif_backer(struct dpif_backer *backer)
>      free(backer->type);
>      free(backer->dp_version_string);
>      dpif_close(backer->dpif);
> +    id_pool_destroy(backer->meter_ids);
>      free(backer);
>  }
>
> @@ -787,6 +788,15 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp)
>          = check_variable_length_userdata(backer);
>      backer->dp_version_string = dpif_get_dp_version(backer->dpif);
>
> +    /* Manage Datpath meter IDs if supported. */
> +    struct ofputil_meter_features features;
> +    dpif_meter_get_features(backer->dpif, &features);
> +    if (features.max_meters) {
> +        backer->meter_ids = id_pool_create(0, features.max_meters);
> +    } else {
> +        backer->meter_ids = NULL;
> +    }
> +
>      return error;
>  }
>
> @@ -5385,6 +5395,17 @@ meter_set(struct ofproto *ofproto_, ofproto_meter_id *meter_id,
>  {
>      struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>
> +    /* Provider ID unknown. Use backer to allocate a new DP meter */
> +    if (meter_id->uint32 == UINT32_MAX) {
> +        if (!ofproto->backer->meter_ids) {
> +            return EFBIG; /* Datapath does not support meter.  */
> +        }
> +
> +        if(!id_pool_alloc_id(ofproto->backer->meter_ids, &meter_id->uint32)) {
> +            return ENOMEM; /* Can't allocate a DP meter. */
> +        }
> +    }
> +
>      switch (dpif_meter_set(ofproto->backer->dpif, meter_id, config)) {
>      case 0:
>          return 0;
> @@ -5414,12 +5435,31 @@ meter_get(const struct ofproto *ofproto_, ofproto_meter_id meter_id,
>      return OFPERR_OFPMMFC_UNKNOWN_METER;
>  }
>
> +struct free_meter_id_args {
> +    struct ofproto_dpif *ofproto;
> +    ofproto_meter_id meter_id;
> +};
> +
> +static void
> +free_meter_id(struct free_meter_id_args *args)
> +{
> +    struct ofproto_dpif *ofproto = args->ofproto;
> +
> +    dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
> +    id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
> +    free(args);
> +}
> +
>  static void
>  meter_del(struct ofproto *ofproto_, ofproto_meter_id meter_id)
>  {
> -    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> +    struct free_meter_id_args *arg = xmalloc(sizeof *arg);
>
> -    dpif_meter_del(ofproto->backer->dpif, meter_id, NULL, 0);
> +    /* The 'meter_id' may still be in use the xlate code.
> +     * Only safe to delet after RCU grace period. */
> +    arg->ofproto = ofproto_dpif_cast(ofproto_);
> +    arg->meter_id = meter_id;
> +    ovsrcu_postpone(free_meter_id, arg);
>  }
>
>  const struct ofproto_class ofproto_dpif_class = {
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 81a0bdfdad10..d14ab62ae195 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -51,6 +51,7 @@
>  #include "hmapx.h"
>  #include "odp-util.h"
>  #include "openvswitch/ofp-util.h"
> +#include "id-pool.h"
>  #include "ovs-thread.h"
>  #include "ofproto-provider.h"
>  #include "util.h"
> @@ -221,6 +222,9 @@ struct dpif_backer {
>
>      bool recv_set_enable; /* Enables or disables receiving packets. */
>
> +    /* Meter. */
> +    struct id_pool *meter_ids;     /* Datapath meter allocation. */
> +
>      /* Version string of the datapath stored in OVSDB. */
>      char *dp_version_string;
>
> --
> 1.8.3.1
>

Patch hide | download patch | download mbox

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a14a2ebb5b2a..d5417162b7af 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -260,7 +260,6 @@  struct dp_netdev {
     /* Meters. */
     struct ovs_mutex meter_locks[N_METER_LOCKS];
     struct dp_meter *meters[MAX_METERS]; /* Meter bands. */
-    uint32_t meter_free;                 /* Next free meter. */
 
     /* Protects access to ofproto-dpif-upcall interface during revalidator
      * thread synchronization. */
@@ -3896,9 +3895,6 @@  dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id,
     struct dp_meter *meter;
     int i;
 
-    if (mid == UINT32_MAX) {
-        mid = dp->meter_free;
-    }
     if (mid >= MAX_METERS) {
         return EFBIG; /* Meter_id out of range. */
     }
@@ -3958,21 +3954,6 @@  dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id,
         dp->meters[mid] = meter;
         meter_unlock(dp, mid);
 
-        meter_id->uint32 = mid; /* Store on success. */
-
-        /* Find next free meter */
-        if (dp->meter_free == mid) { /* Now taken. */
-            do {
-                if (++mid >= MAX_METERS) { /* Wrap around */
-                    mid = 0;
-                }
-                if (mid == dp->meter_free) { /* Full circle */
-                    mid = MAX_METERS;
-                    break;
-                }
-            } while (dp->meters[mid]);
-            dp->meter_free = mid; /* Next free meter or MAX_METERS */
-        }
         return 0;
     }
     return ENOMEM;
@@ -4027,11 +4008,6 @@  dpif_netdev_meter_del(struct dpif *dpif,
         meter_lock(dp, meter_id);
         dp_delete_meter(dp, meter_id);
         meter_unlock(dp, meter_id);
-
-        /* Keep free meter index as low as possible */
-        if (meter_id < dp->meter_free) {
-            dp->meter_free = meter_id;
-        }
     }
     return error;
 }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6a5ffb94fa94..a026d4913731 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -662,6 +662,7 @@  close_dpif_backer(struct dpif_backer *backer)
     free(backer->type);
     free(backer->dp_version_string);
     dpif_close(backer->dpif);
+    id_pool_destroy(backer->meter_ids);
     free(backer);
 }
 
@@ -787,6 +788,15 @@  open_dpif_backer(const char *type, struct dpif_backer **backerp)
         = check_variable_length_userdata(backer);
     backer->dp_version_string = dpif_get_dp_version(backer->dpif);
 
+    /* Manage Datpath meter IDs if supported. */
+    struct ofputil_meter_features features;
+    dpif_meter_get_features(backer->dpif, &features);
+    if (features.max_meters) {
+        backer->meter_ids = id_pool_create(0, features.max_meters);
+    } else {
+        backer->meter_ids = NULL;
+    }
+
     return error;
 }
 
@@ -5385,6 +5395,17 @@  meter_set(struct ofproto *ofproto_, ofproto_meter_id *meter_id,
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
 
+    /* Provider ID unknown. Use backer to allocate a new DP meter */
+    if (meter_id->uint32 == UINT32_MAX) {
+        if (!ofproto->backer->meter_ids) {
+            return EFBIG; /* Datapath does not support meter.  */
+        }
+
+        if(!id_pool_alloc_id(ofproto->backer->meter_ids, &meter_id->uint32)) {
+            return ENOMEM; /* Can't allocate a DP meter. */
+        }
+    }
+
     switch (dpif_meter_set(ofproto->backer->dpif, meter_id, config)) {
     case 0:
         return 0;
@@ -5414,12 +5435,31 @@  meter_get(const struct ofproto *ofproto_, ofproto_meter_id meter_id,
     return OFPERR_OFPMMFC_UNKNOWN_METER;
 }
 
+struct free_meter_id_args {
+    struct ofproto_dpif *ofproto;
+    ofproto_meter_id meter_id;
+};
+
+static void
+free_meter_id(struct free_meter_id_args *args)
+{
+    struct ofproto_dpif *ofproto = args->ofproto;
+
+    dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
+    id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
+    free(args);
+}
+
 static void
 meter_del(struct ofproto *ofproto_, ofproto_meter_id meter_id)
 {
-    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+    struct free_meter_id_args *arg = xmalloc(sizeof *arg);
 
-    dpif_meter_del(ofproto->backer->dpif, meter_id, NULL, 0);
+    /* The 'meter_id' may still be in use the xlate code.
+     * Only safe to delet after RCU grace period. */
+    arg->ofproto = ofproto_dpif_cast(ofproto_);
+    arg->meter_id = meter_id;
+    ovsrcu_postpone(free_meter_id, arg);
 }
 
 const struct ofproto_class ofproto_dpif_class = {
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 81a0bdfdad10..d14ab62ae195 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -51,6 +51,7 @@ 
 #include "hmapx.h"
 #include "odp-util.h"
 #include "openvswitch/ofp-util.h"
+#include "id-pool.h"
 #include "ovs-thread.h"
 #include "ofproto-provider.h"
 #include "util.h"
@@ -221,6 +222,9 @@  struct dpif_backer {
 
     bool recv_set_enable; /* Enables or disables receiving packets. */
 
+    /* Meter. */
+    struct id_pool *meter_ids;     /* Datapath meter allocation. */
+
     /* Version string of the datapath stored in OVSDB. */
     char *dp_version_string;