[ovs-dev] [PATCH v2 11/13] datapath: Remove memory allocations from ovs_flow_cmd_execute.

Jarno Rajahalme jrajahalme at nicira.com
Thu Feb 13 18:54:32 UTC 2014


I had an offline discussion on this with Pravin. I wasn’t fully aware of the kernel stack restrictions, and as this specific patch seemed to have little impact, we’ll drop this for now.

  Jarno

On Feb 12, 2014, at 3:48 PM, Pravin Shelar <pshelar at nicira.com> wrote:

> On Tue, Feb 11, 2014 at 4:07 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
>> ---
>> datapath/datapath.c |   60 +++++++++++++++++++++++++++------------------------
>> 1 file changed, 32 insertions(+), 28 deletions(-)
>> 
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index a46ceb0..90fdc60 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -492,14 +492,24 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>> {
>>        struct ovs_header *ovs_header = info->userhdr;
>>        struct nlattr **a = info->attrs;
>> -       struct sw_flow_actions *acts;
>>        struct sk_buff *packet;
>> -       struct sw_flow *flow;
>>        struct datapath *dp;
>>        struct ethhdr *eth;
>> +       struct sw_flow flow;
>> +       struct {
>> +               struct sw_flow_actions acts;
>> +               u64 buf[512 / sizeof (u64)];
>> +       } sw_acts;
>> +       struct sw_flow_actions *acts;
>>        int len;
>>        int err;
>> 
> 
> I am not sure about stack usage here. Specifically on platforms with
> 4KB stack we can overrun kernel stack.
> 
>> +       /* Start with actions in the stack with enough space for most cases. */
>> +       acts = &sw_acts.acts;
>> +       acts->actions_len = 0;
>> +       acts->alloc_size = sizeof(sw_acts);
>> +       acts->alloced = false;
>> +
>>        err = -EINVAL;
>>        if (!a[OVS_PACKET_ATTR_PACKET] || !a[OVS_PACKET_ATTR_KEY] ||
>>            !a[OVS_PACKET_ATTR_ACTIONS])
>> @@ -525,54 +535,48 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>>        else
>>                packet->protocol = htons(ETH_P_802_2);
>> 
>> -       /* Build an sw_flow for sending this packet. */
>> -       flow = ovs_flow_alloc();
>> -       err = PTR_ERR(flow);
>> -       if (IS_ERR(flow))
>> -               goto err_kfree_skb;
>> -
>> -       err = ovs_flow_extract(packet, -1, &flow->key);
>> +       /* Build an sw_flow for sending this packet.
>> +        * We do not initialize mask, nor stats as they are not used
>> +        * by a flow that is not in the flow table. */
>> +       err = ovs_flow_extract(packet, -1, &flow.key);
>>        if (err)
>> -               goto err_flow_free;
>> +               goto err_free;
>> 
>> -       err = ovs_nla_get_flow_metadata(flow, a[OVS_PACKET_ATTR_KEY]);
>> +       err = ovs_nla_get_flow_metadata(&flow, a[OVS_PACKET_ATTR_KEY]);
>>        if (err)
>> -               goto err_flow_free;
>> -       acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_PACKET_ATTR_ACTIONS]));
>> -       err = PTR_ERR(acts);
>> -       if (IS_ERR(acts))
>> -               goto err_flow_free;
>> +               goto err_free;
>> 
>>        err = ovs_nla_copy_actions(a[OVS_PACKET_ATTR_ACTIONS],
>> -                                  &flow->key, 0, &acts);
>> -       rcu_assign_pointer(flow->sf_acts, acts);
>> +                                  &flow.key, 0, &acts);
>> +       rcu_assign_pointer(flow.sf_acts, acts);
>>        if (err)
>> -               goto err_flow_free;
>> +               goto err_free_acts;
>> 
>> -       OVS_CB(packet)->flow = flow;
>> -       OVS_CB(packet)->pkt_key = &flow->key;
>> -       packet->priority = flow->key.phy.priority;
>> -       packet->mark = flow->key.phy.skb_mark;
>> +       OVS_CB(packet)->flow = &flow;
>> +       OVS_CB(packet)->pkt_key = &flow.key;
>> +       packet->priority = flow.key.phy.priority;
>> +       packet->mark = flow.key.phy.skb_mark;
>> 
>>        rcu_read_lock();
>>        dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>>        err = -ENODEV;
>>        if (!dp)
>>                goto err_unlock;
>> -
>>        local_bh_disable();
>>        err = ovs_execute_actions(dp, packet);
>>        local_bh_enable();
>>        rcu_read_unlock();
>> 
>> -       ovs_flow_free(flow, false);
>> +       /* Will free if actions were realloced. */
>> +       ovs_nla_free_flow_actions(acts, false);
>> +
>>        return err;
>> 
>> err_unlock:
>>        rcu_read_unlock();
>> -err_flow_free:
>> -       ovs_flow_free(flow, false);
>> -err_kfree_skb:
>> +err_free_acts:
>> +       ovs_nla_free_flow_actions(acts, false);
>> +err_free:
>>        kfree_skb(packet);
>> err:
>>        return err;
>> --
>> 1.7.10.4
>> 
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140213/0d91aec5/attachment-0003.html>


More information about the dev mailing list