[ovs-dev] [PATCH 1/2] OVN: add meter support to trigger_event action

Mark Michelson mmichels at redhat.com
Fri Aug 9 15:20:49 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.

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;
>           }
> 



More information about the dev mailing list