Hey Jarno, I addressed all the comments in this and your other email (also the offline discussion about BUILD_ASSERT_DECL) in the following patch v6 (same as v5 but an updated commit message).
Cheers, Ryan On 6/3/14 6:41 PM, "Jarno Rajahalme" <jrajaha...@nicira.com> wrote: > >> On Jun 3, 2014, at 6:22 PM, Ryan Wilson 76511 <wr...@vmware.com> wrote: >> >> Sure, I combined them and sent out a new patch fixing all the bugs I >> encountered in the DPDK perf test. >> >> >>https://urldefense.proofpoint.com/v1/url?u=http://patchwork.openvswitch.o >>rg/patch/4388/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbf >>fg%3D%3D%0A&m=zQcTtDpIgcajJFt34DzsVfFoLH%2FiHqkVEk5mWQGZ5FM%3D%0A&s=05f22 >>e48cfa1b36644528518e00db0dbdc789717db758d4f84e2d55e99050d80 >> >> >> 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(). > >ofpbuf_delete() is supposed to delete the ofpbuf itself, while >ofpbuf_ininit() deletes any associated storage for an ofpbuf whose own >memory is managed elsewhere (stack, some struct, etc). > >Upcall handling will call uninit on a copy of the dpdk ofpbuf, not on the >dpdk allocated buf itself, so the ofpbuf pointer given as a parameter to >uninit remains valid even after the call. > >> I'm not sure I entirely like that since we're freeing the header too, >> which goes directly against the philosophy of ofpbuf_uninit(). >> > >As said above, you should be fine in this regard. > >> 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. >> > >An assert to this effect would help detect/avoid bugs. > >> 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.openvswitc >>>>>h. >>>>> >>>>>org/patch/4375/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXi >>>>>dh >>>>> >>>>>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 >> >>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman >>/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbff >>g%3D%3D%0A&m=zQcTtDpIgcajJFt34DzsVfFoLH%2FiHqkVEk5mWQGZ5FM%3D%0A&s=2f4f8f >>d7fde0c6f78c0016defa274efae807293f186c595d3c069da0e58cf1bc _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
