> 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. > > 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().
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.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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
