[ovs-dev] [PATCH ovn] controller: add datapath meter capability check

[Lorenzo Bianconi]

> 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>