[ovs-dev] [PATCH v2] datapath: Increase maximum allocation size of action list.

Jesse Gross jesse at nicira.com
Sat Feb 23 00:22:16 UTC 2013


On Fri, Feb 22, 2013 at 3:10 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/flow.c b/datapath/flow.c
> index b14229f..e8e1e14 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -207,14 +207,33 @@ struct sw_flow_actions *ovs_flow_actions_alloc(int size)
>         if (size > MAX_ACTIONS_BUFSIZE)
>                 return ERR_PTR(-EINVAL);
>
> -       sfa = kmalloc(sizeof(*sfa) + size, GFP_KERNEL);
> -       if (!sfa)
> -               return ERR_PTR(-ENOMEM);
> +       size += sizeof(*sfa);
> +       if (size <= MAX_ACTIONS_BUFSIZE_KMALLOC) {
> +               sfa = kmalloc(size, GFP_KERNEL);
> +               if (!sfa)
> +                       return ERR_PTR(-ENOMEM);
> +
> +               sfa->actions_len = 0;
> +               sfa->size = ksize(sfa);

I think this can potentially (although probably unlikely) cause a
problem.  Since ksize could be larger than the size we requested, it's
possible that the difference could cross the boundary between kmalloc
and vmalloc, causing us to use the wrong one later.  I would just use
the requested size and then we can also combine the logic other than
allocation for both kmalloc and vmalloc.

> +void ovs_flow_actions_free(struct sw_flow_actions *sfa)
> +{
> +       if (sfa->size <= MAX_ACTIONS_BUFSIZE)

Should this be MAX_ACTIONS_BUFSIZE_KMALLOC?

> diff --git a/datapath/flow.h b/datapath/flow.h
> index 6949640..376e127 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -37,6 +37,7 @@ struct sk_buff;
>  struct sw_flow_actions {
>         struct rcu_head rcu;
>         u32 actions_len;
> +       int size;

I might call this buf_size or similar to be more specific.

> @@ -221,6 +223,7 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *table,
>  void ovs_flow_tbl_destroy(struct flow_table *table);
>  void ovs_flow_tbl_deferred_destroy(struct flow_table *table);
>  struct flow_table *ovs_flow_tbl_alloc(int new_size);
> +void ovs_flow_actions_free(struct sw_flow_actions *sfa);

Can you group this above with the flow alloc function?



More information about the dev mailing list