[ovs-dev] [PATCH 3/3] datapath: refactor do_output() to move skb_clone NULL check out of fast path

[Andy Zhou]

Thanks Pravin for the review. I have pushed all 3 patches with the
suggested changes. Also merged the first patch into branch 2.3.

On Thu, Jul 10, 2014 at 2:04 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Thu, Jul 10, 2014 at 1:16 AM, Andy Zhou <azhou at nicira.com> wrote:
>> skb_clone() NULL check is implemented in do_output(), as past of the
>> common (fast) path. Refactoring so that NULL check is done in the
>> slow path, immediately after skb_clone() is called.
>>
>> Besides optimization, this change also improves code readability by
>> making the skb_clone() NULL check consistent within OVS datapath
>> module.
>>
>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>> ---
>>  datapath/actions.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index daa7b43..6eb83b8 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -490,9 +490,6 @@ static int do_output(struct datapath *dp, struct sk_buff *skb, int out_port)
>>  {
>>         struct vport *vport;
>>
>> -       if (unlikely(!skb))
>> -               return -ENOMEM;
>> -
> There is no check for do_output() return, Therefore we can make it return void.
>
>
>>         vport = ovs_vport_rcu(dp, out_port);
>>         if (unlikely(!vport)) {
>>                 kfree_skb(skb);
>> @@ -693,7 +690,12 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>                 int err = 0;
>>
>>                 if (prev_port != -1) {
> This can be marked as unlikely
>
>> -                       do_output(dp, skb_clone(skb, GFP_ATOMIC), prev_port);
>> +                       struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC);
>> +
>> +                       if (!out_skb)
>> +                               return -ENOMEM;
>> +
> This changes behavior, we can just skip do_output if it skb_clone() fails.
>
>> +                       do_output(dp, out_skb, prev_port);
>>                         prev_port = -1;
>>                 }
>>
> otherwise looks good.