> On Apr 14, 2017, at 12:46 PM, Andy Zhou <az...@ovn.org> wrote:
>
> Add 'meter_ids', an id-pool object to manage datapath 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 <az...@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. */
->”Datapath"
> + 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.
What about megaflows still using the meter_id, is that a problem? I guess it is
OK for the meter to not be found during action execution, but how is situation
different from the translation code referring to a stale meter_id?
> + * Only safe to delet after RCU grace period. */
->”delete”
> + 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
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
