[ovs-dev] [PATCH] dpif-netdev: Remove an extra memcpy of packet data from datapath-upcall interface for bridges with datapath_type=netdev.

[Pravin Shelar]

Can you combine it with second patch, otherwise it introduces a bug.

On Tue, Jun 3, 2014 at 4:22 PM, Ryan Wilson 76511 <wryan at vmware.com> wrote:
> I've ran into some unexpected issues while perf testing this, lets hold
> off on looking at this. I'll submit another patch when I've had all the
> kinks worked out.
>
> Ryan
>
> On 6/3/14 2:21 PM, "Ryan Wilson 76511" <wryan at vmware.com> wrote:
>
>>Hey Pravin,
>>
>>Thanks for the catch here! Turns out the header is already tracked in DPDK
>>with rte_mbuf's buffer address - sizeof(ofpbuf). Thus, I submitted another
>>patch that, in free_dpdk_buf(), always gets this header and uses this to
>>free memory.
>>
>>http://patchwork.openvswitch.org/patch/4375/
>>
>>
>>Let me know if this patch works.
>>
>>Cheers,
>>
>>Ryan
>>
>>On 6/3/14 10:50 AM, "Pravin Shelar" <pshelar at nicira.com> wrote:
>>
>>>On Tue, Jun 3, 2014 at 10:33 AM, Ryan Wilson <wryan at nicira.com> wrote:
>>>> When a bridge of datatype type netdev receives a packet, it copies the
>>>> packet from the NIC to a buffer in userspace. Currently, when making
>>>> an upcall, the packet is again copied to the upcall's buffer. However,
>>>> this extra copy is not necessary when the datapath exists in userspace
>>>> as the upcall can directly access the packet data.
>>>>
>>>> This patch eliminates this extra copy of the packet data in most cases.
>>>> In cases where the packet may still be used later by callers of
>>>> dp_netdev_execute_actions, making a copy of the packet data is still
>>>> necessary.
>>>>
>>>> Signed-off-by: Ryan Wilson <wryan at nicira.com>
>>>> ---
>>>> v2: Addressed Jarno's comment to use direct pointer assignment for
>>>> upcall->packet instead of ofpbuf_set().
>>>> ---
>>>>  lib/dpif-netdev.c |   15 +++++----------
>>>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>>> index 91c83d6..c89ae20 100644
>>>> --- a/lib/dpif-netdev.c
>>>> +++ b/lib/dpif-netdev.c
>>>> @@ -2024,7 +2024,6 @@ dp_netdev_input(struct dp_netdev *dp, struct
>>>>ofpbuf *packet,
>>>>                                     miniflow_hash_5tuple(&key.flow, 0)
>>>>                                     % dp->n_handlers,
>>>>                                     DPIF_UC_MISS, &key.flow, NULL);
>>>> -        ofpbuf_delete(packet);
>>>>      }
>>>>  }
>>>>
>>>> @@ -2063,7 +2062,6 @@ dp_netdev_output_userspace(struct dp_netdev *dp,
>>>>struct ofpbuf *packet,
>>>>          if (userdata) {
>>>>              buf_size += NLA_ALIGN(userdata->nla_len);
>>>>          }
>>>> -        buf_size += ofpbuf_size(packet);
>>>>          ofpbuf_init(buf, buf_size);
>>>>
>>>>          /* Put ODP flow. */
>>>> @@ -2078,15 +2076,14 @@ dp_netdev_output_userspace(struct dp_netdev
>>>>*dp, struct ofpbuf *packet,
>>>>
>>>>NLA_ALIGN(userdata->nla_len));
>>>>          }
>>>>
>>>> -        ofpbuf_set_data(&upcall->packet,
>>>> -                        ofpbuf_put(buf, ofpbuf_data(packet),
>>>>ofpbuf_size(packet)));
>>>> -        ofpbuf_set_size(&upcall->packet, ofpbuf_size(packet));
>>>> +        upcall->packet = *packet;
>>>>
>>>
>>>This would not work with DPDK. ofpbuf from dpdk is special case where
>>>ofpbuf and data are allocated from same memory object. Therefore
>>>moving ofpbuf->data is nontrivial.
>>>
>>>To make it work we need atleast following covered.
>>>1. Define flag for source of ofpbuf header. So that we can track
>>>header and data independently.
>>>2. Fix dpdk ofpbuf destructor to free correct dpdk memory object.
>>>
>>>Thanks,
>>>Pravin.
>>
>