| Message ID | 1493370083-7385-2-git-send-email-azhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
With a note below: Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Apr 28, 2017, at 2:01 AM, 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> > > --- > 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." 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. 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Apr 28, 2017 at 12:47 PM, Jarno Rajahalme <jarno@ovn.org> wrote: > With a note below: > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > >> On Apr 28, 2017, at 2:01 AM, 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> >> >> --- >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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. */ + 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, 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> --- 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(-)