lkl tools: virtio net fd: make it posix compatible#202
lkl tools: virtio net fd: make it posix compatible#20210 commits merged intomasterfrom unknown repository
Conversation
|
Thanks @opurdila - I gave a quick try and #195 looks like alleviated (I didn't reproduce hangs so far). I will review more carefully this PR later. |
Review status: 0 of 12 files reviewed at latest revision, 6 unresolved discussions. tools/lkl/include/lkl_host.h, line 101 [r1] (raw file):
releated => related tools/lkl/lib/virtio_net.c, line 146 [r1] (raw file):
do we want to handle if ret < 0 ? tools/lkl/lib/virtio_net_fd.c, line 46 [r1] (raw file):
missing blank line tools/lkl/lib/virtio_net_fd.c, line 69 [r1] (raw file):
missing blank line tools/lkl/lib/virtio_net_fd.c, line 107 [r1] (raw file):
missing blank line tools/lkl/lib/virtio_net_fd.c, line 162 [r1] (raw file):
line feed (see checkpatch.pl) Comments from Reviewable |
|
This looks like a great simplification of what was some very ugly code! FWIW with the hijack issue, you can use the workaround I had for I guess this avoids the unspecified behavior of closing an fd that's being polled since it relies on the write side rather than the read side? |
|
Reviewed 11 of 13 files at r1. tools/lkl/lib/virtio_net.c, line 289 [r1] (raw file):
So who free dev-nd now? tools/lkl/lib/virtio_net_fd.c, line 84 [r1] (raw file):
Is there race condition on poll_mask? It's accessed/modified by poll_thread and lkl kernel thread without synchronization. Comments from Reviewable |
|
@pscollins I feel that this sort of workaround is trying to solve the problem in the wrong place. We should try to fix it somehow in the hijack library - for example by checking if LKL is running and if not use native pipe instead of LKL pipe. I have it on my TODO list, and hope to have something soon. @thehajime Unfortunately there is no POSIX edge-triggered primitive we can use. Also, there is no fundamental difference between edge and level, we can easily emulate edge with level by making sure to read / write until we get -EAGAIN. So at the moment I don't see any reason to try use edge-trigger notifications. Review status: 6 of 12 files reviewed at latest revision, 8 unresolved discussions. tools/lkl/include/lkl_host.h, line 101 [r1] (raw file):
|
|
Reviewed 2 of 6 files at r2. tools/lkl/lib/virtio_net.c, line 267 [r3] (raw file):
I'm seeing the following error after running netperf tens of times. I doubt if it's because we called lkl_netdev_remove before lkl_sys_halt [ 11.800000] ------------[ cut here ]------------ tools/lkl/lib/virtio_net_fd.c, line 115 [r3] (raw file):
Should we inititialize events to 0? tools/lkl/lib/virtio_net_fd.c, line 178 [r3] (raw file):
it's better to use host_ops->mem_alloc? because you are using host_ops->free when closing it. Comments from Reviewable |
|
I've pushed a new version is adds a few more cleanups and it also splits the changes to make them easier to review. Review status: 3 of 16 files reviewed at latest revision, 11 unresolved discussions. tools/lkl/lib/virtio_net.c, line 267 [r3] (raw file):
|
ghost
mentioned this pull request
|
Review status: 3 of 16 files reviewed at latest revision, 6 unresolved discussions. tools/lkl/lib/virtio_net.c, line 267 [r3] (raw file):
|
struct lkl_netdev has been changed to include an additional field and thus the current casts in VDE don't work anymore. Fix this by including struct lkl_netdev into struct lkl_netdev_vde and using container_of instead of plain casts. Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Collapse the rx and tx poll threads into a single poll thread. I did not notice any performance degradation and reducing the number of threads reduces the complexity. Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
This allows us to identify poll errors and make it a bit more explicit when the poll thread needs to exit. It also allows us to move the poll thread join to virtio_net.c to simplify implementation for other backends. It also makes the thread create and join operations are symmetric as both are now happening in virtio_net.c Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Add a new form on interceptors that issue host system calls if LKL has not yet started. This is to be used for those system calls that are not easy to determine if they belong to LKL or not (such as pipe or eventfd). Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Rework the virtio net Linux fd implementation to make it POSIX compatible. We do this by removing dependencies on epoll and eventfd. Instead of using eventfd to shutdown the poll thread we use a control pipe - closing the write side of the pipe will issue a POLLHUP and wake-up the poll thread. Like-wise, instead of using an epoll edge-triggered approach to mitigate the high CPU usage on the TX side, we use a level-triggered poll approach where the poll POLLIN / POLLOUT mask is set only when is needed, i.e. when the read or write calls returns EAGAIN. The control pipe is used to wake-up the poll thread and change the poll mask to avoid race between the poll thread and LKL threads issuing reads or writes. Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
There is no need to duplicate the net device operations in both virtio_net_dev and lkl_netdev. Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Now that we have a reliable way of shutting down the virtio net poll thread we can (a) convert the close operation "return" void and (b) remove the lkl_netdevs_remove() call from lkl_sys_halt. This patch also makes lkl_netdev_remove() available to be called by the application before lkl_sys_halt. This makes the lkl_netdev_add/remove operations symmetric and easier to use by applications. Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
|
Reviewed 1 of 13 files at r1, 3 of 5 files at r3, 9 of 10 files at r4, 2 of 2 files at r5. a discussion (no related file): tools/lkl/lib/virtio_net_fd.c, line 48 [r5] (raw file):
Why not just use tools/lkl/lib/virtio_net_fd.c, line 137 [r5] (raw file):
Shouldn't we really return HUP in the tools/lkl/lib/virtio_net_macvtap.c, line 19 [r5] (raw file):
This header isn't in POSIX, for example :-). tools/lkl/lib/hijack/hijack.c, line 158 [r5] (raw file):
Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 12 unresolved discussions. a discussion (no related file):
|
|
Review status: all files reviewed at latest revision, 11 unresolved discussions. tools/lkl/lib/virtio_net_fd.c, line 48 [r5] (raw file):
|
|
Review status: all files reviewed at latest revision, 10 unresolved discussions. tools/lkl/lib/virtio_net_fd.c, line 48 [r5] (raw file):
|
|
Review status: all files reviewed at latest revision, 10 unresolved discussions. tools/lkl/lib/hijack/hijack.c, line 158 [r5] (raw file):
|
|
I didn't see any difference in performance from my testing. Review status: all files reviewed at latest revision, 10 unresolved discussions. Comments from Reviewable |
ghost
merged commit
|
@opurdila, After this change, |
4 participants
Rework the virtio net Linux fd implementation to make it POSIX
compatible. We do this by removing dependencies on epoll and eventfd.
Instead of using eventfd to shutdown the poll thread we use a control
pipe - closing the write side of the pipe will issue a POLLHUP and
wake-up the poll thread.
Note that this patch uses pipe2 instead of pipe in order to address a
libhijack limitation where pipe is hijacked regardless of calls coming
from the library or not. This makes virtio_net_fd Linux specific, but
we can easily switch back to pipe once the libhijack issue is resolved.
Like-wise, instead of using an epoll edge-triggered approach to
mitigate the high CPU usage on the TX side, we use a level-triggered
poll approach where the poll POLLIN / POLLOUT mask is set only when is
needed, i.e. when the read or write calls returns EAGAIN. The control
pipe is used to wake-up the poll thread and change the poll mask to
avoid race between the poll thread and LKL threads issuing reads or
writes.
Also collapse the rx and tx poll threads into a single poll thread. I
did not notice any performance degradation and reducing the number of
threads reduces the complexity.
Since the the poll thread can now be stopped reliably the patch also
removes lkl_netdevs_remove() and makes lkl_netdev_remove() available
to make the lkl_netdev_add/remove operations
symmetric. lkl_netdev_remove() can be called by the application before
lkl_sys_halt so we can also remove it from lkl_sys_halt.
Signed-off-by: Octavian Purdila octavian.purdila@intel.com
This change is