Sure, I combined them and sent out a new patch fixing all the bugs I encountered in the DPDK perf test.
http://patchwork.openvswitch.org/patch/4388/ One issue is that since ofproto-dpif-upcall calls ofpbuf_uninit(), I had to free the DPDK buffer in that function as opposed to ofpbuf_delete(). I'm not sure I entirely like that since we're freeing the header too, which goes directly against the philosophy of ofpbuf_uninit(). One idea I had was to only allow ofpbuf_uninit to be called with OFPBUF_DPDK only if struct ofpbuf *b != ofpbuf_header(b) (meaning b is a copy of the header and thus we can free the original header). I figured I'd let you check out the patch and see if you had any other ideas. Cheers, Ryan On 6/3/14 5:22 PM, "Pravin Shelar" <pshe...@nicira.com> wrote: >Can you combine it with second patch, otherwise it introduces a bug. > >On Tue, Jun 3, 2014 at 4:22 PM, Ryan Wilson 76511 <wr...@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" <wr...@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. >>> >>>https://urldefense.proofpoint.com/v1/url?u=http://patchwork.openvswitch. >>>org/patch/4375/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidh >>>bffg%3D%3D%0A&m=gZZtvgxp3GIUGlkz85Olmu3cqs62R0tf%2F4IxSAQLQ9Y%3D%0A&s=20 >>>9e52cdfc433e5fa4deb7ef48a561f78207981f0dabf4dd5ecb3257bde613e2 >>> >>> >>>Let me know if this patch works. >>> >>>Cheers, >>> >>>Ryan >>> >>>On 6/3/14 10:50 AM, "Pravin Shelar" <pshe...@nicira.com> wrote: >>> >>>>On Tue, Jun 3, 2014 at 10:33 AM, Ryan Wilson <wr...@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 <wr...@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. >>> >> _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
