lkl tools: introduce Intel DPDK network backend for virtio device #67

thehajime · 2016-02-06T14:22:38Z

This involves several refactors in lkl tools:

split tap code from hijack library to general library
change env name for interface name of tap
old) LKL_HIJACK_NET_TAP
new) LKL_HIJACK_NET_IFTYPE and LKL_HIJACK_NET_IFNAME
use opqaue pointers for lkl_netdev.
Using opaque pointers remove the implementation details from lkl.h
and increase the scalability for additional device emulation backends.
expose common test functions and macros into tests/test.h

only tested with dpdk-2.2.0, vmxnet3 driver. at least e1000 and e1000e
don't work on vmware fusion (6.0.x).

to build with dpdk backend, one needs to do in advance:

./scripts/dpdk-sdk-build.sh
make dpdk=yes

Signed-off-by: Thomas Liebetraut thomas@tommie-lie.de
Signed-off-by: Hajime Tazaki thehajime@gmail.com

thehajime · 2016-02-06T14:26:17Z

the way to specify to build in DPDK is an open question.
there are a couple of choices: currently the former was implemented.

make argument (make dpdk=yes)
CONFIG_LKL_DPDK like option ? (configured by make xxxconfig ARCH=lkl)

thehajime · 2016-02-19T04:51:24Z

thanks @stfairy for the comment. I'll get you back addressing your comments (I think I'm fine:)).

thehajime · 2016-02-19T09:04:31Z

rebased and split into commits.

for additional information how to use dpdk, I drafted a wiki page, which could be put in lkl/linux later.

https://github.com/libos-nuse/lkl-linux/wiki/DPDK-Howto-(tmp)

pscollins · 2016-02-23T00:06:02Z

Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

tools/lkl/lib/virtio_net_dpdk.c, line 64 [r1] (raw file):
Is this threadsafe? Does it require any synchronization?

tools/lkl/lib/virtio_net_dpdk.c, line 83 [r1] (raw file):
Ditto --- threadsafe?

tools/lkl/lib/virtio_net_dpdk.c, line 97 [r1] (raw file):
What's this sleep for?

tools/lkl/lib/virtio_net_dpdk.c, line 103 [r1] (raw file):
No need for rm0 here, just assign straight to rm

tools/lkl/lib/virtio_net_dpdk.c, line 117 [r1] (raw file):
I imagine this is a fairly regular occurrence, you probably want to wrap it in an #ifdef DEBUG (or define a fprintf equivalent that's wrapped like that)

tools/lkl/lib/virtio_net_dpdk.c, line 149 [r1] (raw file):
Why doesn't this call poll(2)? net_enqueue in tools/lkl/lib/virtio_net.c expects this to be reasonably honest, because it doesn't want to block on a read/write call and hold on to a lock for longer than needed.

tools/lkl/lib/virtio_net_dpdk.c, line 196 [r1] (raw file):
I think you should use NULL if you're returning a pointer; it's more intention-revealing

tools/lkl/lib/virtio_net_dpdk.c, line 271 [r1] (raw file):
IMO this should be a compile-time error, with the #error pragma

tools/lkl/lib/virtio_net_tap.c, line 5 [r1] (raw file):
You might want to add at @opurdila here

tools/lkl/lib/virtio_net_tap.c, line 24 [r1] (raw file):
Upstream has changed so these aren't needed anymore, you might want to rebase on to it

tools/lkl/lib/virtio_net_tap.c, line 40 [r1] (raw file):
I'm not sure if it's easily available here, but the container_of macro in .e.g include/lkl/linux/virtio_net.h is a nice way to do this that allows for struct members to be reordered down the line.

tools/lkl/Makefile, line 47 [r1] (raw file):
IMO this should be something like CONFIG_LKL_DPDK

tools/lkl/scripts/dpdk-sdk-build.sh, line 5 [r1] (raw file):
I don't think you should clone git repos as part of the build process --- maybe add it as a submodule? Someone who knows their build processes better than I do should weigh in --- @tommie-lie?

tools/lkl/tests/net-test.c, line 112 [r1] (raw file):
It's not safe to do this until after you've checked argc

tools/lkl/tests/net-test.c, line 195 [r1] (raw file):
endif /* __MIGW32__ */

tools/lkl/tests/net-test.c, line 204 [r1] (raw file):
endif /* __MIGW32__ */

Though it looks like you might not want this file to be compiled at all if we're on Windows?

tools/lkl/tests/net.sh, line 11 [r1] (raw file):
Where are the DPDK tests run? Are they hard to do in a platform-independent way?

Comments from the review on Reviewable.io

thehajime · 2016-02-24T11:45:05Z

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

tools/lkl/lib/virtio_net_dpdk.c, line 64 [r1] (raw file):
good catch. some reported DPDK's mempool with cache is not thread safe (e.g., http://www.dpdk.io/ml/archives/dev/2014-February/001401.html). not sure if it's already fixed or not. so I will disable the cache by changing MEMPOOL_CACHE_SZ to 0.

other than that, I don't see any worries on thread safeness.

tools/lkl/lib/virtio_net_dpdk.c, line 83 [r1] (raw file):
nd_dpdk->rms is potentially not safe in multiple threads.

if virtio_process_one() is called by multiple threads then this should be guarded.

tools/lkl/lib/virtio_net_dpdk.c, line 97 [r1] (raw file):
see my another comments on poll(2) below.

tools/lkl/lib/virtio_net_dpdk.c, line 103 [r1] (raw file):
okay. will fix it.

tools/lkl/lib/virtio_net_dpdk.c, line 117 [r1] (raw file):
okay. will wrap with ifdef DEBUG.

tools/lkl/lib/virtio_net_dpdk.c, line 149 [r1] (raw file):
dpdk's interrupt mode has equivalent of epoll_wait(2), which we can apply here. but AFAIK the mode is only available on limited NIC drivers like ixgbe/igb/e1000 (with dpdk v2.2.0), while my test environment uses vmxnet3.

sleep() after rte_eth_rx_burst needs for this reason to relax processor.

so overall of this patch, I tried to be available as much drivers as possible.

tools/lkl/lib/virtio_net_dpdk.c, line 196 [r1] (raw file):
thanks. will fix those.

tools/lkl/lib/virtio_net_dpdk.c, line 271 [r1] (raw file):
since this function is called via library (sometime via LD_PRELOAD with configuration of env variable), giving helpful message to a library user isn't harmful IMHO.

e.g.,

sudo LKL_HIJACK_NET_VIF=dpdk LKL_HIJACK_NET_IFNAME=dpdk LKL_HIJACK_NET_IP=192.168.13.2 LKL_HIJACK_NET_NETMASK_LEN=24  LKL_HIJACK_DEBUG=1 ./bin/lkl-hijack.sh ip ad

since we/I don't have any cool manager of each VIF right now, doing it in each VIF is the way to be polite. what do you think ?

tools/lkl/lib/virtio_net_tap.c, line 5 [r1] (raw file):
indeed, thanks !

tools/lkl/lib/virtio_net_tap.c, line 24 [r1] (raw file):
will fix it. thanks.

tools/lkl/lib/virtio_net_tap.c, line 40 [r1] (raw file):
let me think a bit more for this: I will keep this opened.
(though I agree with you for the usefulness of container_of macro)

tools/lkl/Makefile, line 47 [r1] (raw file):
will see how #82 reaches a consensus.

tools/lkl/scripts/dpdk-sdk-build.sh, line 5 [r1] (raw file):
this script is never called from build system: it is intended by people by hand in advance, with the help of wiki document.

and I think having git submodule in linux tree is hard to convince again so, I just crafted this script for a work around.

tools/lkl/tests/net-test.c, line 112 [r1] (raw file):
okay. will fix it.

tools/lkl/tests/net-test.c, line 204 [r1] (raw file):
right, as lib/virtio_net* are filtered out in tools/lkl/Makefile on mingw32 build.

tools/lkl/tests/net.sh, line 11 [r1] (raw file):
no idea for non-linux platform at this moment.
and preparations to use dpdk are complicated as documented in wiki.

so I don't see any benefit to do a test suite.

Comments from the review on Reviewable.io

pscollins · 2016-02-24T18:22:49Z

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

tools/lkl/lib/virtio_net_dpdk.c, line 64 [r1] (raw file):
Okay --- it might be nice to stick in a comment explaining when it could be dangerous to use (in the TAP case, it's easy to tell that read(2) or write(2) are okay, but it's less obvious here).

tools/lkl/lib/virtio_net_dpdk.c, line 83 [r1] (raw file):
Ditto, please stick in a comment so that e.g. after a future refactor that allows parallel writes, if something breaks it will be easier to figure out where to start. Also, virtio_process_one can be called by 2 threads concurrently (1 per queue), so be aware of that.

tools/lkl/lib/virtio_net_dpdk.c, line 149 [r1] (raw file):
Hmmm, okay, that's unfortunate. Oh well, nothing to be done about it I guess.

tools/lkl/lib/virtio_net_dpdk.c, line 271 [r1] (raw file):
Makes sense. In my head I imagined there was a way to make it so this symbol isn't available unless LKL was compiled with DPDK, but I guess that's not the case (for now, at least).

tools/lkl/tests/net.sh, line 11 [r1] (raw file):
Well, I guess what I really meant to say was "Don't we want this running on circleCI?" because I worry that it will get broken and no one will notice until after the fact. If you don't think it's worth it, then no worries.

Comments from the review on Reviewable.io

xjia1 · 2016-03-04T02:20:55Z

Sorry I didn't notice there were some comments on Reviewable so maybe some of my comments were duplicate.

Aside from that, can you also split this pull request into two, so your first refactoring commit can get merged early before people can fully review the DPDK part?

The refactoring looks good to me except for a few minor details.

Thanks,
Xiao

thehajime · 2016-03-04T04:50:25Z

Aside from that, can you also split this pull request into two, so your first refactoring commit can get merged early before people can fully review the DPDK part?

agree: will keep dpdk part to this PR and split refactor part to new PR.

thehajime · 2016-03-07T05:12:11Z

I split tap refactor part to another PR (#118): please comment the tap part to the PR if you have, not in this PR.

thehajime · 2016-03-08T23:29:07Z

Reviewed 1 of 7 files at r2.
Review status: 3 of 20 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

tools/lkl/lib/virtio_net_dpdk.c, line 64 [r1] (raw file):
let me know if the current commit addresses your request.

tools/lkl/lib/virtio_net_dpdk.c, line 83 [r1] (raw file):
this too: let me how you feel with new comment.

tools/lkl/tests/net-test.c, line 195 [r1] (raw file):
added.

tools/lkl/tests/net.sh, line 11 [r1] (raw file):
I added a dummy test in tests/net.sh for DPDK so that users can customize the test. it is not likely to do a generic test in all environment. if someone comes up with a nice test for DPDK, it would be great but I have no idea.

on CircleCI, neither tap nor DPDK are available. more likely way would be use of raw socket with a root privilege.

Comments from the review on Reviewable.io

pscollins · 2016-03-10T18:58:02Z

Reviewed 1 of 17 files at r3, 1 of 5 files at r4.
Review status: 4 of 14 files reviewed at latest revision, 2 unresolved discussions.

tools/lkl/lib/virtio_net_dpdk.c, line 64 [r1] (raw file):
Great!

tools/lkl/lib/virtio_net_dpdk.c, line 83 [r1] (raw file):
Perfect!

Comments from the review on Reviewable.io

This commit also adds a test for networking (tests/net-test.c) using same framework as boot.c. As a result, it exposes common test functions and macros into tests/test.h. only tested with dpdk-2.2.0, vmxnet3 driver. at least e1000 and e1000e don't work on vmware fusion (6.0.x). to build with dpdk backend, one needs to do in advance: - ./scripts/dpdk-sdk-build.sh - make dpdk=yes Signed-off-by: Hajime Tazaki <thehajime@gmail.com>

thehajime · 2016-03-11T02:41:45Z

I modified a bit (the name of ifdefs) and rebased it.

lkl tools: introduce Intel DPDK network backend for virtio device

thehajime · 2016-03-14T05:37:28Z

I created a wiki page (https://github.com/lkl/linux/wiki/Howto:-DPDK-with-LKL) for the DPDK introduction.

thehajime force-pushed the upstream-dpdk branch 2 times, most recently from 7622e45 to b950200 Compare February 19, 2016 07:21

thehajime added the enhancement label Feb 19, 2016

This was referenced Feb 20, 2016

Is LKL being used somewhere for production use? #79

Closed

Reorganize build system and make host library configurable #82

Closed

thehajime force-pushed the upstream-dpdk branch from b950200 to bd29e90 Compare February 24, 2016 11:45

thehajime mentioned this pull request Feb 26, 2016

document for environmental variables in Documentation/lkl.txt #94

Closed

thehajime force-pushed the upstream-dpdk branch 2 times, most recently from 035462e to 9f84ec8 Compare March 7, 2016 05:07

thehajime mentioned this pull request Mar 7, 2016

lkl tools: refactor tap code from hijack library to general library #118

Merged

thehajime force-pushed the upstream-dpdk branch from 9f84ec8 to 2b435e4 Compare March 8, 2016 23:29

thehajime force-pushed the upstream-dpdk branch from 2b435e4 to 229b941 Compare March 11, 2016 02:39

ghost pushed a commit that referenced this pull request Mar 11, 2016

Merge pull request #67 from libos-nuse/upstream-dpdk

553417a

lkl tools: introduce Intel DPDK network backend for virtio device

ghost merged commit 553417a into lkl:master Mar 11, 2016

thehajime deleted the upstream-dpdk branch March 14, 2016 05:36

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lkl tools: introduce Intel DPDK network backend for virtio device #67

lkl tools: introduce Intel DPDK network backend for virtio device #67

thehajime commented Feb 6, 2016

thehajime commented Feb 6, 2016

thehajime commented Feb 19, 2016

thehajime commented Feb 19, 2016

pscollins commented Feb 23, 2016

thehajime commented Feb 24, 2016

pscollins commented Feb 24, 2016

xjia1 commented Mar 4, 2016

thehajime commented Mar 4, 2016

thehajime commented Mar 7, 2016

thehajime commented Mar 8, 2016

pscollins commented Mar 10, 2016

thehajime commented Mar 11, 2016

thehajime commented Mar 14, 2016

lkl tools: introduce Intel DPDK network backend for virtio device #67

lkl tools: introduce Intel DPDK network backend for virtio device #67

Conversation

thehajime commented Feb 6, 2016

thehajime commented Feb 6, 2016

thehajime commented Feb 19, 2016

thehajime commented Feb 19, 2016

pscollins commented Feb 23, 2016

thehajime commented Feb 24, 2016

pscollins commented Feb 24, 2016

xjia1 commented Mar 4, 2016

thehajime commented Mar 4, 2016

thehajime commented Mar 7, 2016

thehajime commented Mar 8, 2016

pscollins commented Mar 10, 2016

thehajime commented Mar 11, 2016

thehajime commented Mar 14, 2016