[ovs-dev] [PATCH 1/2] OVN: add meter support to trigger_event action
Lorenzo Bianconi
lorenzo.bianconi at redhat.com
Fri Aug 9 17:36:11 UTC 2019
>
> I noticed this patch introduced a memory leak since event->meter is
> never freed.
>
> However, I can understand how this happened because currently everything
> in ovnact_controller_event is being leaked.
> ovnact_controller_event_free() should be calling free_gen_options(), but
> it's not. And with this patch, it should now also free event->meter.
>
> To avoid convoluting the purpose of each patch, I think the easiest way
> to handle this is to submit a v2, where the first patch fixes the
> existing memory leak. Then patch 2 can be this one, with the added
> freeing of event->meter. And the current patch 2 of this changeset can
> become patch 3.
Hi Mark,
ack, I agree. I will post v2 soon. Thanks.
Regards,
Lorenzo
>
> The memory leak fix will need to be backported to OVS 2.12 as well.
>
> On 8/1/19 5:56 AM, Lorenzo Bianconi wrote:
> > Introduce meter support to trigger_event action in order to not
> > overload pinctrl thread under heavy load
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > ---
> > include/ovn/actions.h | 1 +
> > lib/actions.c | 42 ++++++++++++++++++++++++++++++++++++------
> > 2 files changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 63d3907d8..1f2ac2eb2 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -326,6 +326,7 @@ struct ovnact_controller_event {
> > int event_type; /* controller event type */
> > struct ovnact_gen_option *options;
> > size_t n_options;
> > + char *meter;
> > };
> >
> > /* Internal use by the helpers below. */
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 4eacc44ed..2e321d82e 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -1273,6 +1273,9 @@ format_TRIGGER_EVENT(const struct ovnact_controller_event *event,
> > {
> > ds_put_format(s, "trigger_event(event = \"%s\"",
> > event_to_string(event->event_type));
> > + if (event->meter) {
> > + ds_put_format(s, ", meter = \"%s\"", event->meter);
> > + }
> > for (const struct ovnact_gen_option *o = event->options;
> > o < &event->options[event->n_options]; o++) {
> > ds_put_cstr(s, ", ");
> > @@ -1420,10 +1423,21 @@ encode_TRIGGER_EVENT(const struct ovnact_controller_event *event,
> > const struct ovnact_encode_params *ep OVS_UNUSED,
> > struct ofpbuf *ofpacts)
> > {
> > + uint32_t meter_id = NX_CTLR_NO_METER;
> > size_t oc_offset;
> >
> > + if (event->meter) {
> > + meter_id = ovn_extend_table_assign_id(ep->meter_table, event->meter,
> > + ep->lflow_uuid);
> > + if (meter_id == EXT_TABLE_ID_INVALID) {
> > + VLOG_WARN("Unable to assign id for trigger meter: %s",
> > + event->meter);
> > + return;
> > + }
> > + }
> > +
> > oc_offset = encode_start_controller_op(ACTION_OPCODE_EVENT, false,
> > - NX_CTLR_NO_METER, ofpacts);
> > + meter_id, ofpacts);
> > ovs_be32 ofs = htonl(event->event_type);
> > ofpbuf_put(ofpacts, &ofs, sizeof ofs);
> >
> > @@ -1738,11 +1752,27 @@ parse_trigger_event(struct action_context *ctx,
> > sizeof *event->options);
> > }
> >
> > - struct ovnact_gen_option *o = &event->options[event->n_options++];
> > - memset(o, 0, sizeof *o);
> > - parse_gen_opt(ctx, o,
> > - &ctx->pp->controller_event_opts->event_opts[event_type],
> > - event_to_string(event_type));
> > + if (lexer_match_id(ctx->lexer, "meter")) {
> > + if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> > + return;
> > + }
> > + /* If multiple meters are given, use the most recent. */
> > + if (ctx->lexer->token.type == LEX_T_STRING &&
> > + strlen(ctx->lexer->token.s)) {
> > + free(event->meter);
> > + event->meter = xstrdup(ctx->lexer->token.s);
> > + } else if (ctx->lexer->token.type != LEX_T_STRING) {
> > + lexer_syntax_error(ctx->lexer, "expecting string");
> > + return;
> > + }
> > + lexer_get(ctx->lexer);
> > + } else {
> > + struct ovnact_gen_option *o = &event->options[event->n_options++];
> > + memset(o, 0, sizeof *o);
> > + parse_gen_opt(ctx, o,
> > + &ctx->pp->controller_event_opts->event_opts[event_type],
> > + event_to_string(event_type));
> > + }
> > if (ctx->lexer->error) {
> > return;
> > }
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list