| Message ID | ec06ddea3673bd7affcf1e248c549b0befaa9cf3.1629275497.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] controller: add datapath meter capability check | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 18/08/2021 09:32, Lorenzo Bianconi wrote: > Dump datapath meter capabilities before configuring meters in > ovn-controller > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > controller/lflow.c | 10 +++++++++- > controller/lflow.h | 3 +++ > controller/pinctrl.c | 10 ++++++++++ > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 923d8f0a4..7d5b987cf 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -47,6 +47,8 @@ COVERAGE_DEFINE(lflow_run); > /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ > static struct shash symtab; > > +static bool lflow_dp_meter = true; > + > void > lflow_init(void) > { > @@ -648,7 +650,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > .ct_snat_vip_ptable = OFTABLE_CT_SNAT_HAIRPIN, > .fdb_ptable = OFTABLE_GET_FDB, > .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB, > - .ctrl_meter_id = ctrl_meter_id, > + .ctrl_meter_id = lflow_dp_meter ? ctrl_meter_id : NX_CTLR_NO_METER, > }; > ovnacts_encode(ovnacts->data, ovnacts->size, &ep, &ofpacts); > > @@ -762,6 +764,12 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, > return expr_simplify(e); > } > > +void I think you need *some* kind of synchronization here. Maybe not a mutex but perhaps an atomic read/write. > +lflow_meter_supported(bool val) > +{ > + lflow_dp_meter = val; > +} > + > static bool > consider_logical_flow__(const struct sbrec_logical_flow *lflow, > const struct sbrec_datapath_binding *dp, > diff --git a/controller/lflow.h b/controller/lflow.h > index 61ffbc0cc..d22c39861 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -191,4 +191,7 @@ bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *, > struct lflow_ctx_out *); > bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *, > struct lflow_ctx_out *); > + > +void lflow_meter_supported(bool val); > + > #endif /* controller/lflow.h */ > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index cc3edaaf4..f9d453ad2 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -39,6 +39,7 @@ > #include "openvswitch/ofp-switch.h" > #include "openvswitch/ofp-util.h" > #include "openvswitch/vlog.h" > +#include "openvswitch/ofp-meter.h" > #include "lib/random.h" > #include "lib/crc32c.h" > > @@ -549,6 +550,10 @@ queue_msg(struct rconn *swconn, struct ofpbuf *msg) > static void > pinctrl_setup(struct rconn *swconn) > { > + /* dump datapath meter capabilities. */ > + queue_msg(swconn, ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST, > + rconn_get_version(swconn), 0)); > + Is pinctrl the right place to do this? Should this not be done in the main controller thread on start up? > /* Fetch the switch configuration. The response later will allow us to > * change the miss_send_len to UINT16_MAX, so that we can enable > * asynchronous messages. */ > @@ -3258,6 +3263,11 @@ pinctrl_recv(struct rconn *swconn, const struct ofp_header *oh, > set_switch_config(swconn, &config); > } else if (type == OFPTYPE_PACKET_IN) { > process_packet_in(swconn, oh); > + } else if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) { > + struct ofputil_meter_features mf; > + > + ofputil_decode_meter_features(oh, &mf); > + lflow_meter_supported(mf.max_meters > 0); > } else { > if (VLOG_IS_DBG_ENABLED()) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300); >
> On 18/08/2021 09:32, Lorenzo Bianconi wrote: > > Dump datapath meter capabilities before configuring meters in > > ovn-controller > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > controller/lflow.c | 10 +++++++++- > > controller/lflow.h | 3 +++ > > controller/pinctrl.c | 10 ++++++++++ > > 3 files changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 923d8f0a4..7d5b987cf 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -47,6 +47,8 @@ COVERAGE_DEFINE(lflow_run); > > /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ > > static struct shash symtab; > > > > +static bool lflow_dp_meter = true; > > + > > void > > lflow_init(void) > > { > > @@ -648,7 +650,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > > .ct_snat_vip_ptable = OFTABLE_CT_SNAT_HAIRPIN, > > .fdb_ptable = OFTABLE_GET_FDB, > > .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB, > > - .ctrl_meter_id = ctrl_meter_id, > > + .ctrl_meter_id = lflow_dp_meter ? ctrl_meter_id : NX_CTLR_NO_METER, > > }; > > ovnacts_encode(ovnacts->data, ovnacts->size, &ep, &ofpacts); > > > > @@ -762,6 +764,12 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, > > return expr_simplify(e); > > } > > > > +void > > I think you need *some* kind of synchronization here. Maybe not a mutex > but perhaps an atomic read/write. Hi Mark, Thx for the review. ack, I will use atomic_bool here. > > > +lflow_meter_supported(bool val) > > +{ > > + lflow_dp_meter = val; > > +} > > + > > static bool > > consider_logical_flow__(const struct sbrec_logical_flow *lflow, > > const struct sbrec_datapath_binding *dp, > > diff --git a/controller/lflow.h b/controller/lflow.h > > index 61ffbc0cc..d22c39861 100644 > > --- a/controller/lflow.h > > +++ b/controller/lflow.h > > @@ -191,4 +191,7 @@ bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *, > > struct lflow_ctx_out *); > > bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *, > > struct lflow_ctx_out *); > > + > > +void lflow_meter_supported(bool val); > > + > > #endif /* controller/lflow.h */ > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index cc3edaaf4..f9d453ad2 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -39,6 +39,7 @@ > > #include "openvswitch/ofp-switch.h" > > #include "openvswitch/ofp-util.h" > > #include "openvswitch/vlog.h" > > +#include "openvswitch/ofp-meter.h" > > #include "lib/random.h" > > #include "lib/crc32c.h" > > > > @@ -549,6 +550,10 @@ queue_msg(struct rconn *swconn, struct ofpbuf *msg) > > static void > > pinctrl_setup(struct rconn *swconn) > > { > > + /* dump datapath meter capabilities. */ > > + queue_msg(swconn, ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST, > > + rconn_get_version(swconn), 0)); > > + > > Is pinctrl the right place to do this? Should this not be done in the > main controller thread on start up? I added it here just to reuse the ovs-vswtichd connection managament, but we can move it to the main controller thread (in this case we can probably even just use a bool since the variable will be mostly read only). I will work on a v2. Regrads, Lorenzo > > > /* Fetch the switch configuration. The response later will allow us to > > * change the miss_send_len to UINT16_MAX, so that we can enable > > * asynchronous messages. */ > > @@ -3258,6 +3263,11 @@ pinctrl_recv(struct rconn *swconn, const struct ofp_header *oh, > > set_switch_config(swconn, &config); > > } else if (type == OFPTYPE_PACKET_IN) { > > process_packet_in(swconn, oh); > > + } else if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) { > > + struct ofputil_meter_features mf; > > + > > + ofputil_decode_meter_features(oh, &mf); > > + lflow_meter_supported(mf.max_meters > 0); > > } else { > > if (VLOG_IS_DBG_ENABLED()) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300); > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/lflow.c b/controller/lflow.c index 923d8f0a4..7d5b987cf 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -47,6 +47,8 @@ COVERAGE_DEFINE(lflow_run); /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ static struct shash symtab; +static bool lflow_dp_meter = true; + void lflow_init(void) { @@ -648,7 +650,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, .ct_snat_vip_ptable = OFTABLE_CT_SNAT_HAIRPIN, .fdb_ptable = OFTABLE_GET_FDB, .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB, - .ctrl_meter_id = ctrl_meter_id, + .ctrl_meter_id = lflow_dp_meter ? ctrl_meter_id : NX_CTLR_NO_METER, }; ovnacts_encode(ovnacts->data, ovnacts->size, &ep, &ofpacts); @@ -762,6 +764,12 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, return expr_simplify(e); } +void +lflow_meter_supported(bool val) +{ + lflow_dp_meter = val; +} + static bool consider_logical_flow__(const struct sbrec_logical_flow *lflow, const struct sbrec_datapath_binding *dp, diff --git a/controller/lflow.h b/controller/lflow.h index 61ffbc0cc..d22c39861 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -191,4 +191,7 @@ bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *, struct lflow_ctx_out *); bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *, struct lflow_ctx_out *); + +void lflow_meter_supported(bool val); + #endif /* controller/lflow.h */ diff --git a/controller/pinctrl.c b/controller/pinctrl.c index cc3edaaf4..f9d453ad2 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -39,6 +39,7 @@ #include "openvswitch/ofp-switch.h" #include "openvswitch/ofp-util.h" #include "openvswitch/vlog.h" +#include "openvswitch/ofp-meter.h" #include "lib/random.h" #include "lib/crc32c.h" @@ -549,6 +550,10 @@ queue_msg(struct rconn *swconn, struct ofpbuf *msg) static void pinctrl_setup(struct rconn *swconn) { + /* dump datapath meter capabilities. */ + queue_msg(swconn, ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST, + rconn_get_version(swconn), 0)); + /* Fetch the switch configuration. The response later will allow us to * change the miss_send_len to UINT16_MAX, so that we can enable * asynchronous messages. */ @@ -3258,6 +3263,11 @@ pinctrl_recv(struct rconn *swconn, const struct ofp_header *oh, set_switch_config(swconn, &config); } else if (type == OFPTYPE_PACKET_IN) { process_packet_in(swconn, oh); + } else if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) { + struct ofputil_meter_features mf; + + ofputil_decode_meter_features(oh, &mf); + lflow_meter_supported(mf.max_meters > 0); } else { if (VLOG_IS_DBG_ENABLED()) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
Dump datapath meter capabilities before configuring meters in ovn-controller Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- controller/lflow.c | 10 +++++++++- controller/lflow.h | 3 +++ controller/pinctrl.c | 10 ++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-)