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

[Pravin Shelar]

On Fri, Feb 22, 2013 at 4:22 PM, Jesse Gross <jesse at nicira.com> wrote:
> 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.
>
I do not think it can happen if KMALLOC limit is same as a kmalloc cache size.
but I agree it is bit safer to use size.

I will send updated patch.

>> +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?