| Message ID | 1492199182-4479-3-git-send-email-azhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
> On 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. > > 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. */ ->”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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Apr 27, 2017 at 3:23 PM, Jarno Rajahalme <jarno@ovn.org> wrote: > >> On 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. >> >> 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. */ > > ->”Datapath" Thanks, Fixed. > >> + 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? Good question, Here is how I think about it: I believe the patch does fix a race condition in the user space. from openflow view point, all rules should have been deleted before the meter is deleted. So the fix should be complete from use space view point. You are right in pointing out additional issue w.r.t. datapath sync. All rules being deleted does not guarantee the corresponding megaflow does not exist in the datapath. One possible solution is to delay id_pool_free_id() call further until all referencing megaflow has been invalidated. Another possibility is for datapath model to handle it with some sort of ref counting. At any rate, I don't believe the patch makes the situation any worst than it is now. > >> + * Only safe to delet after RCU grace period. */ > > ->”delete” Thanks fixed. > >> + 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Apr 27, 2017, at 11:48 PM, Andy Zhou <azhou@ovn.org> wrote: > > On Thu, Apr 27, 2017 at 3:23 PM, Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>> wrote: >> >>> On 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. >>> >>> 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. */ >> >> ->”Datapath" > Thanks, Fixed. >> >>> + 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? > > Good question, Here is how I think about it: > > I believe the patch does fix a race condition in the user space. from > openflow view point, all rules should have been deleted > before the meter is deleted. So the fix should be complete from use > space view point. > > You are right in pointing out additional issue w.r.t. datapath sync. > All rules being deleted does not guarantee the corresponding > megaflow does not exist in the datapath. One possible solution is to > delay id_pool_free_id() call further until all referencing megaflow > has been invalidated. Another possibility is for datapath model to > handle it with some sort of ref counting. At any rate, I don't > believe the patch makes the situation any worst than it is now. > I guess the only real problem here would be that the freed meter_id would be immediately reused and old megaflows would then use the new meter (instead of old one). One solution to avoid that is to (immediately) queue to-be-freed id allocations to a queue, and then from revalidator move that queue to another (queue2) after a full revalidation. After next full revalidation the items in queue2 can be freed. This guarantees that a full revalidation of all the megaflows has been completed before the provider meter id is reused. We do something similar with recirculation IDs, for similar purposes. Jarno >> >>> + * Only safe to delet after RCU grace period. */ >> >> ->”delete” > Thanks fixed. >> >>> + 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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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;
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 <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(-)