On Fri, Apr 28, 2017 at 12:47 PM, Jarno Rajahalme <[email protected]> wrote:
> With a note below:
>
> Acked-by: Jarno Rajahalme <[email protected]>
>
>> On Apr 28, 2017, at 2:01 AM, Andy Zhou <[email protected]> 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 <[email protected]>
>>
>> ---
>> v1-v2: fix typos
>> ---
>> 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 b3a080628d2b..f4de737e3751 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 c73c2738c91c..30f18b302a77 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 Datapath 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;
>> }
>>
>> @@ -5439,6 +5449,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;
>> @@ -5468,12 +5489,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 delete after RCU grace period. */
>
> I’d like to see this comment expanded a bit, e.g., “Even when rules referring
> to a meter are deleted, those rules, and their actions, remain accessible to
> upcall translation for upcalls whose processing started before the meter was
> deleted."
Thanks. I expended the comment, and fixed other minor issues
mentioned, and pushed the series to master.
>
> Also, in the future we may want to sync id recycling with the revalidation
> cycle so that we know the ID to be freed is not used by datapath flows any
> more.
>
Yes, sync with datapath needs more work. Noted.
> Jarno
>
>> + 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
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
