| Message ID | 1492199182-4479-4-git-send-email-azhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
With small nits below: Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Apr 14, 2017, at 12:46 PM, Andy Zhou <azhou@ovn.org> wrote: > > Allow action upcall meters, i.e. slowpath and controller meters, > to be added and displayed. > > Keep track of datapath meter ID of those action upcall meters in > ofproto to aid action translation. Later patches will make use of them. > > Signed-off-by: Andy Zhou <azhou@ovn.org> > --- > lib/ofp-print.c | 33 ++++++++++++++++++++++++++--- > ofproto/ofproto-provider.h | 4 ++++ > ofproto/ofproto.c | 52 ++++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 82 insertions(+), 7 deletions(-) > > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > index a8cdfcbf20b1..140af05950b7 100644 > --- a/lib/ofp-print.c > +++ b/lib/ofp-print.c > @@ -1333,11 +1333,36 @@ ofp_print_meter_band(struct ds *s, uint16_t flags, > } > > static void > +ofp_print_meter_id(struct ds *s, uint32_t meter_id, char seperator) > +{ > + if (meter_id <= OFPM13_MAX) { > + ds_put_format(s, "meter%c%"PRIu32, seperator, meter_id); > + } else { > + const char *name; > + switch (meter_id) { > + case OFPM13_SLOWPATH: > + name = "slowpath"; > + break; > + case OFPM13_CONTROLLER: > + name = "controller"; > + break; > + case OFPM13_ALL: > + name = "ALL”; We require lower case “all” when parsing, so better print that way also. > + break; > + default: > + name = "unknown"; > + } > + ds_put_format(s, "meter%c%s", seperator, name); > + } > +} > + > +static void > ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms) > { > uint16_t i; > > - ds_put_format(s, "meter:%"PRIu32" ", ms->meter_id); > + ofp_print_meter_id(s, ms->meter_id, ':'); > + ds_put_char(s, ' '); > ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count); > ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count); > ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count); > @@ -1358,7 +1383,8 @@ ofp_print_meter_config(struct ds *s, const struct ofputil_meter_config *mc) > { > uint16_t i; > > - ds_put_format(s, "meter=%"PRIu32" ", mc->meter_id); > + ofp_print_meter_id(s, mc->meter_id, '='); > + ds_put_char(s, ' '); > > ofp_print_meter_flags(s, mc->flags); > > @@ -1412,8 +1438,9 @@ ofp_print_meter_stats_request(struct ds *s, const struct ofp_header *oh) > uint32_t meter_id; > > ofputil_decode_meter_request(oh, &meter_id); > + ds_put_char(s, ' '); > > - ds_put_format(s, " meter=%"PRIu32, meter_id); > + ofp_print_meter_id(s, meter_id, '='); > } > > static const char * > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 000326d7f79d..688a9e5d32eb 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -112,6 +112,10 @@ struct ofproto { > /* Meter table. */ > struct ofputil_meter_features meter_features; > struct hmap meters; /* uint32_t indexed 'struct meter *'. */ > + uint32_t slowpath_meter_id; /* Datapath slowpath meter. UINT32_MAX > + if not defined. */ > + uint32_t controller_meter_id; /* Datapath controller meter. UINT32_MAX > + if not defined. */ > > /* OpenFlow connections. */ > struct connmgr *connmgr; > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 8c4c7e67f213..abbb849a384b 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -568,6 +568,8 @@ ofproto_create(const char *datapath_name, const char *datapath_type, > memset(&ofproto->meter_features, 0, sizeof ofproto->meter_features); > } > hmap_init(&ofproto->meters); > + ofproto->slowpath_meter_id = UINT32_MAX; > + ofproto->controller_meter_id = UINT32_MAX; > > /* Set the initial tables version. */ > ofproto_bump_tables_version(ofproto); > @@ -6232,9 +6234,33 @@ ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id) > return NULL; > } > > +static uint32_t * > +ofproto_upcall_meter_ptr(struct ofproto *ofproto, uint32_t meter_id) > +{ > + switch(meter_id) { > + case OFPM13_SLOWPATH: > + return &ofproto->slowpath_meter_id; > + break; > + case OFPM13_CONTROLLER: > + return &ofproto->controller_meter_id; > + break; > + case OFPM13_ALL: > + OVS_NOT_REACHED(); > + default: > + return NULL; > + } > +} > + > static void > ofproto_add_meter(struct ofproto *ofproto, struct meter *meter) > { > + uint32_t *upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id); > + > + /* Cache datapath meter IDs of special meters. */ > + if (upcall_meter_ptr) { > + *upcall_meter_ptr = meter->provider_meter_id.uint32; > + } > + > hmap_insert(&ofproto->meters, &meter->node, hash_int(meter->id, 0)); > } > > @@ -6248,7 +6274,7 @@ static bool > ofproto_fix_meter_action(const struct ofproto *ofproto, > struct ofpact_meter *ma) > { > - if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) { > + if (ma->meter_id) { This change would better be placed with patch 1/5? > const struct meter *meter = ofproto_get_meter(ofproto, ma->meter_id); > > if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) { > @@ -6306,6 +6332,12 @@ static void > meter_destroy(struct ofproto *ofproto, struct meter *meter) > OVS_REQUIRES(ofproto_mutex) > { > + uint32_t *upcall_meter_ptr; > + upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id); > + if (upcall_meter_ptr) { > + *upcall_meter_ptr = UINT32_MAX; > + } > + > if (!ovs_list_is_empty(&meter->rules)) { > struct rule_collection rules; > struct rule *rule; > @@ -6438,12 +6470,24 @@ handle_meter_mod(struct ofconn *ofconn, const struct ofp_header *oh) > > if (mm.command != OFPMC13_DELETE) { > /* Fails also when meters are not implemented by the provider. */ > - if (meter_id == 0 || meter_id > OFPM13_MAX) { > + if (ofproto->meter_features.max_meters == 0) { > error = OFPERR_OFPMMFC_INVALID_METER; > goto exit_free_bands; > - } else if (meter_id > ofproto->meter_features.max_meters) { > - error = OFPERR_OFPMMFC_OUT_OF_METERS; > + } > + > + if (meter_id == 0) { > + error = OFPERR_OFPMMFC_INVALID_METER; > goto exit_free_bands; > + } else if (meter_id > OFPM13_MAX) { > + switch(meter_id) { > + case OFPM13_SLOWPATH: > + case OFPM13_CONTROLLER: > + break; > + case OFPM13_ALL: > + default: > + error = OFPERR_OFPMMFC_INVALID_METER; > + goto exit_free_bands; > + } > } > if (mm.meter.n_bands > ofproto->meter_features.max_bands) { > error = OFPERR_OFPMMFC_OUT_OF_BANDS; > -- > 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:33 PM, Jarno Rajahalme <jarno@ovn.org> wrote: > With small nits below: > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > >> On Apr 14, 2017, at 12:46 PM, Andy Zhou <azhou@ovn.org> wrote: >> >> Allow action upcall meters, i.e. slowpath and controller meters, >> to be added and displayed. >> >> Keep track of datapath meter ID of those action upcall meters in >> ofproto to aid action translation. Later patches will make use of them. >> >> Signed-off-by: Andy Zhou <azhou@ovn.org> >> --- >> lib/ofp-print.c | 33 ++++++++++++++++++++++++++--- >> ofproto/ofproto-provider.h | 4 ++++ >> ofproto/ofproto.c | 52 ++++++++++++++++++++++++++++++++++++++++++---- >> 3 files changed, 82 insertions(+), 7 deletions(-) >> >> diff --git a/lib/ofp-print.c b/lib/ofp-print.c >> index a8cdfcbf20b1..140af05950b7 100644 >> --- a/lib/ofp-print.c >> +++ b/lib/ofp-print.c >> @@ -1333,11 +1333,36 @@ ofp_print_meter_band(struct ds *s, uint16_t flags, >> } >> >> static void >> +ofp_print_meter_id(struct ds *s, uint32_t meter_id, char seperator) >> +{ >> + if (meter_id <= OFPM13_MAX) { >> + ds_put_format(s, "meter%c%"PRIu32, seperator, meter_id); >> + } else { >> + const char *name; >> + switch (meter_id) { >> + case OFPM13_SLOWPATH: >> + name = "slowpath"; >> + break; >> + case OFPM13_CONTROLLER: >> + name = "controller"; >> + break; >> + case OFPM13_ALL: >> + name = "ALL”; > > We require lower case “all” when parsing, so better print that way also. Make sense. Will change. > >> + break; >> + default: >> + name = "unknown"; >> + } >> + ds_put_format(s, "meter%c%s", seperator, name); >> + } >> +} >> + >> +static void >> ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms) >> { >> uint16_t i; >> >> - ds_put_format(s, "meter:%"PRIu32" ", ms->meter_id); >> + ofp_print_meter_id(s, ms->meter_id, ':'); >> + ds_put_char(s, ' '); >> ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count); >> ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count); >> ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count); >> @@ -1358,7 +1383,8 @@ ofp_print_meter_config(struct ds *s, const struct ofputil_meter_config *mc) >> { >> uint16_t i; >> >> - ds_put_format(s, "meter=%"PRIu32" ", mc->meter_id); >> + ofp_print_meter_id(s, mc->meter_id, '='); >> + ds_put_char(s, ' '); >> >> ofp_print_meter_flags(s, mc->flags); >> >> @@ -1412,8 +1438,9 @@ ofp_print_meter_stats_request(struct ds *s, const struct ofp_header *oh) >> uint32_t meter_id; >> >> ofputil_decode_meter_request(oh, &meter_id); >> + ds_put_char(s, ' '); >> >> - ds_put_format(s, " meter=%"PRIu32, meter_id); >> + ofp_print_meter_id(s, meter_id, '='); >> } >> >> static const char * >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index 000326d7f79d..688a9e5d32eb 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -112,6 +112,10 @@ struct ofproto { >> /* Meter table. */ >> struct ofputil_meter_features meter_features; >> struct hmap meters; /* uint32_t indexed 'struct meter *'. */ >> + uint32_t slowpath_meter_id; /* Datapath slowpath meter. UINT32_MAX >> + if not defined. */ >> + uint32_t controller_meter_id; /* Datapath controller meter. UINT32_MAX >> + if not defined. */ >> >> /* OpenFlow connections. */ >> struct connmgr *connmgr; >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index 8c4c7e67f213..abbb849a384b 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -568,6 +568,8 @@ ofproto_create(const char *datapath_name, const char *datapath_type, >> memset(&ofproto->meter_features, 0, sizeof ofproto->meter_features); >> } >> hmap_init(&ofproto->meters); >> + ofproto->slowpath_meter_id = UINT32_MAX; >> + ofproto->controller_meter_id = UINT32_MAX; >> >> /* Set the initial tables version. */ >> ofproto_bump_tables_version(ofproto); >> @@ -6232,9 +6234,33 @@ ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id) >> return NULL; >> } >> >> +static uint32_t * >> +ofproto_upcall_meter_ptr(struct ofproto *ofproto, uint32_t meter_id) >> +{ >> + switch(meter_id) { >> + case OFPM13_SLOWPATH: >> + return &ofproto->slowpath_meter_id; >> + break; >> + case OFPM13_CONTROLLER: >> + return &ofproto->controller_meter_id; >> + break; >> + case OFPM13_ALL: >> + OVS_NOT_REACHED(); >> + default: >> + return NULL; >> + } >> +} >> + >> static void >> ofproto_add_meter(struct ofproto *ofproto, struct meter *meter) >> { >> + uint32_t *upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id); >> + >> + /* Cache datapath meter IDs of special meters. */ >> + if (upcall_meter_ptr) { >> + *upcall_meter_ptr = meter->provider_meter_id.uint32; >> + } >> + >> hmap_insert(&ofproto->meters, &meter->node, hash_int(meter->id, 0)); >> } >> >> @@ -6248,7 +6274,7 @@ static bool >> ofproto_fix_meter_action(const struct ofproto *ofproto, >> struct ofpact_meter *ma) >> { >> - if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) { >> + if (ma->meter_id) { > > This change would better be placed with patch 1/5? Yes, It would also make sense there, I will move it. > >> const struct meter *meter = ofproto_get_meter(ofproto, ma->meter_id); >> >> if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) { >> @@ -6306,6 +6332,12 @@ static void >> meter_destroy(struct ofproto *ofproto, struct meter *meter) >> OVS_REQUIRES(ofproto_mutex) >> { >> + uint32_t *upcall_meter_ptr; >> + upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id); >> + if (upcall_meter_ptr) { >> + *upcall_meter_ptr = UINT32_MAX; >> + } >> + >> if (!ovs_list_is_empty(&meter->rules)) { >> struct rule_collection rules; >> struct rule *rule; >> @@ -6438,12 +6470,24 @@ handle_meter_mod(struct ofconn *ofconn, const struct ofp_header *oh) >> >> if (mm.command != OFPMC13_DELETE) { >> /* Fails also when meters are not implemented by the provider. */ >> - if (meter_id == 0 || meter_id > OFPM13_MAX) { >> + if (ofproto->meter_features.max_meters == 0) { >> error = OFPERR_OFPMMFC_INVALID_METER; >> goto exit_free_bands; >> - } else if (meter_id > ofproto->meter_features.max_meters) { >> - error = OFPERR_OFPMMFC_OUT_OF_METERS; >> + } >> + >> + if (meter_id == 0) { >> + error = OFPERR_OFPMMFC_INVALID_METER; >> goto exit_free_bands; >> + } else if (meter_id > OFPM13_MAX) { >> + switch(meter_id) { >> + case OFPM13_SLOWPATH: >> + case OFPM13_CONTROLLER: >> + break; >> + case OFPM13_ALL: >> + default: >> + error = OFPERR_OFPMMFC_INVALID_METER; >> + goto exit_free_bands; >> + } >> } >> if (mm.meter.n_bands > ofproto->meter_features.max_bands) { >> error = OFPERR_OFPMMFC_OUT_OF_BANDS; >> -- >> 1.8.3.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/ofp-print.c b/lib/ofp-print.c index a8cdfcbf20b1..140af05950b7 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -1333,11 +1333,36 @@ ofp_print_meter_band(struct ds *s, uint16_t flags, } static void +ofp_print_meter_id(struct ds *s, uint32_t meter_id, char seperator) +{ + if (meter_id <= OFPM13_MAX) { + ds_put_format(s, "meter%c%"PRIu32, seperator, meter_id); + } else { + const char *name; + switch (meter_id) { + case OFPM13_SLOWPATH: + name = "slowpath"; + break; + case OFPM13_CONTROLLER: + name = "controller"; + break; + case OFPM13_ALL: + name = "ALL"; + break; + default: + name = "unknown"; + } + ds_put_format(s, "meter%c%s", seperator, name); + } +} + +static void ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms) { uint16_t i; - ds_put_format(s, "meter:%"PRIu32" ", ms->meter_id); + ofp_print_meter_id(s, ms->meter_id, ':'); + ds_put_char(s, ' '); ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count); ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count); ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count); @@ -1358,7 +1383,8 @@ ofp_print_meter_config(struct ds *s, const struct ofputil_meter_config *mc) { uint16_t i; - ds_put_format(s, "meter=%"PRIu32" ", mc->meter_id); + ofp_print_meter_id(s, mc->meter_id, '='); + ds_put_char(s, ' '); ofp_print_meter_flags(s, mc->flags); @@ -1412,8 +1438,9 @@ ofp_print_meter_stats_request(struct ds *s, const struct ofp_header *oh) uint32_t meter_id; ofputil_decode_meter_request(oh, &meter_id); + ds_put_char(s, ' '); - ds_put_format(s, " meter=%"PRIu32, meter_id); + ofp_print_meter_id(s, meter_id, '='); } static const char * diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 000326d7f79d..688a9e5d32eb 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -112,6 +112,10 @@ struct ofproto { /* Meter table. */ struct ofputil_meter_features meter_features; struct hmap meters; /* uint32_t indexed 'struct meter *'. */ + uint32_t slowpath_meter_id; /* Datapath slowpath meter. UINT32_MAX + if not defined. */ + uint32_t controller_meter_id; /* Datapath controller meter. UINT32_MAX + if not defined. */ /* OpenFlow connections. */ struct connmgr *connmgr; diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 8c4c7e67f213..abbb849a384b 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -568,6 +568,8 @@ ofproto_create(const char *datapath_name, const char *datapath_type, memset(&ofproto->meter_features, 0, sizeof ofproto->meter_features); } hmap_init(&ofproto->meters); + ofproto->slowpath_meter_id = UINT32_MAX; + ofproto->controller_meter_id = UINT32_MAX; /* Set the initial tables version. */ ofproto_bump_tables_version(ofproto); @@ -6232,9 +6234,33 @@ ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id) return NULL; } +static uint32_t * +ofproto_upcall_meter_ptr(struct ofproto *ofproto, uint32_t meter_id) +{ + switch(meter_id) { + case OFPM13_SLOWPATH: + return &ofproto->slowpath_meter_id; + break; + case OFPM13_CONTROLLER: + return &ofproto->controller_meter_id; + break; + case OFPM13_ALL: + OVS_NOT_REACHED(); + default: + return NULL; + } +} + static void ofproto_add_meter(struct ofproto *ofproto, struct meter *meter) { + uint32_t *upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id); + + /* Cache datapath meter IDs of special meters. */ + if (upcall_meter_ptr) { + *upcall_meter_ptr = meter->provider_meter_id.uint32; + } + hmap_insert(&ofproto->meters, &meter->node, hash_int(meter->id, 0)); } @@ -6248,7 +6274,7 @@ static bool ofproto_fix_meter_action(const struct ofproto *ofproto, struct ofpact_meter *ma) { - if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) { + if (ma->meter_id) { const struct meter *meter = ofproto_get_meter(ofproto, ma->meter_id); if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) { @@ -6306,6 +6332,12 @@ static void meter_destroy(struct ofproto *ofproto, struct meter *meter) OVS_REQUIRES(ofproto_mutex) { + uint32_t *upcall_meter_ptr; + upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, meter->id); + if (upcall_meter_ptr) { + *upcall_meter_ptr = UINT32_MAX; + } + if (!ovs_list_is_empty(&meter->rules)) { struct rule_collection rules; struct rule *rule; @@ -6438,12 +6470,24 @@ handle_meter_mod(struct ofconn *ofconn, const struct ofp_header *oh) if (mm.command != OFPMC13_DELETE) { /* Fails also when meters are not implemented by the provider. */ - if (meter_id == 0 || meter_id > OFPM13_MAX) { + if (ofproto->meter_features.max_meters == 0) { error = OFPERR_OFPMMFC_INVALID_METER; goto exit_free_bands; - } else if (meter_id > ofproto->meter_features.max_meters) { - error = OFPERR_OFPMMFC_OUT_OF_METERS; + } + + if (meter_id == 0) { + error = OFPERR_OFPMMFC_INVALID_METER; goto exit_free_bands; + } else if (meter_id > OFPM13_MAX) { + switch(meter_id) { + case OFPM13_SLOWPATH: + case OFPM13_CONTROLLER: + break; + case OFPM13_ALL: + default: + error = OFPERR_OFPMMFC_INVALID_METER; + goto exit_free_bands; + } } if (mm.meter.n_bands > ofproto->meter_features.max_bands) { error = OFPERR_OFPMMFC_OUT_OF_BANDS;
Allow action upcall meters, i.e. slowpath and controller meters, to be added and displayed. Keep track of datapath meter ID of those action upcall meters in ofproto to aid action translation. Later patches will make use of them. Signed-off-by: Andy Zhou <azhou@ovn.org> --- lib/ofp-print.c | 33 ++++++++++++++++++++++++++--- ofproto/ofproto-provider.h | 4 ++++ ofproto/ofproto.c | 52 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 82 insertions(+), 7 deletions(-)