Lwip echoserver by lucypa · Pull Request #25 · seL4/camkes

lucypa · 2022-03-18T01:28:20Z

This is a simple 2 component echo server with a driver in one address space and the network stack and echo server in the other. This has been benchmarked and tested on the imx8mm-evk and Haswell3 systems.
Corresponding PRs are:

Test with: seL4/global-components#33, seL4/projects_libs#15, seL4/util_libs#126

Signed-off-by: Lucy <[email protected]>

axel-h · 2022-03-18T10:46:24Z

I've added the "Test with:" in the description, so the github CI run will use these PRs.

axel-h · 2022-03-18T10:50:28Z

apps/lwipserver2/CMakeLists.txt

+find_package(projects_libs REQUIRED)
+
+set(CAmkESCPP ON CACHE BOOL "" FORCE)
+if("${KernelArch}" STREQUAL "x86")


can't we just use the boolean flags

Suggested change

if("${KernelArch}" STREQUAL "x86")

if(KernelArchARM)

...

elseif(KernelArchX86)

...

axel-h · 2022-03-18T10:51:14Z

apps/lwipserver2/CMakeLists.txt

+    set(cpp_define -DKernelArchX86)
+elseif("${KernelArch}" STREQUAL "arm")
+    set(cpp_define -DKernelArchArm)
+endif()


let's give it a clear error message here for anything else and avoid potentially undefined default behavior

else() message(FATAL_ERROR "Unsupported architecture: '${KernelArch}'.") endif()

axel-h · 2022-03-18T10:53:45Z

apps/lwipserver2/CMakeLists.txt

+    LIBS
+    ${libs}
+    LD_FLAGS
+    -Wl,--section-start=.note.gnu.build-id=0x400920


It this just a copy paste from somewhere? Maybe add a comment about this, that this is just optional to set a specific build ID. The kernel is using -Wl,--build-id=none" # Ensure reproducible builds here.

axel-h · 2022-03-18T10:56:27Z

apps/lwipserver2/include/echo/tuning_params.h

+#define TX_BUFS 510
+#define RX_BUFS 510
+
+/* Size used for ethernet buffers */


PLease add the note again that this is the next 2^n for the 1500 byte MTU. I assume that this buffer size is independent of the buffer size that ius set it the other PRs, might be good to mention this in the comment also, so it's clear there are not pitfalls in here.

axel-h · 2022-03-18T10:57:21Z

apps/lwipserver2/include/echo/tuning_params.h

+/* Maximum connected TCP clients */
+#define MAX_TCP_CLIENTS 5
+
+/* Size of initial TCP socket reads */


Can you add a note why exactly this value is chosen? Is it random or related to TCP headers?

axel-h · 2022-03-18T10:57:43Z

apps/lwipserver2/include/echo/tuning_params.h

+/* Size of initial TCP socket reads */
+#define TCP_READ_SIZE 1400
+
+/* Max size of UDP socket reads */


Same here, is it random or related to UDP headers?

axel-h · 2022-03-18T10:58:38Z

apps/lwipserver2/include/echo/tuning_params.h

+#define DMA_ALLOC_SIZE (DMA_RING_ALLOC_SIZE + BUF_SIZE * (TX_BUFS + RX_BUFS))
+
+
+/* Heap size */


Can you add a comment how the heap is used, so there is a rough explanation what influcenses this?

axel-h · 2022-03-18T11:02:46Z

apps/lwipserver2/lwipserver2_imx8mm.camkes

+    attribute int cnode_size_bits = 12;
+    attribute int simple_untyped20_pool = 2;
+    attribute int promiscuous_mode = 1;
+    attribute int heap_size = 0x10000;


Can you add a comment here what drives this heap size?

axel-h · 2022-03-18T11:06:14Z

apps/lwipserver2/lwipserver2_imx8mm.camkes

+    emits FDT resource;
+}
+
+component LWIPServer {


I wonder, is there a way to have one common generic CAmkES file with the server that includes the platform specific driver files? So this can be extended to other platforms easily without copy/pasting common things?

axel-h · 2022-03-18T11:07:20Z

apps/lwipserver2/settings.cmake

+
+# This application only works on hardware at the moment, not simulation
+# set(SIMULATION OFF CACHE BOOL "" FORCE)
+if("${KernelArch}" STREQUAL "x86")


Suggested change

if("${KernelArch}" STREQUAL "x86")

if(KernelArchX86)

axel-h · 2022-03-18T11:08:17Z

apps/lwipserver2/settings.cmake

+    set(KernelIOMMU ON CACHE BOOL "" FORCE)
+endif()
+
+set(cpp_define -DKernelArchArm)


Shouldn't this be set only for KernelArchARM - if this must be set at all?

axel-h · 2022-03-18T11:09:27Z

apps/lwipserver2/src/tcp_echo_socket.c

+
+static ps_io_ops_t *ops;
+
+static int peer_id;


unsigned int?

kent-mcleod · 2022-03-25T06:15:45Z

apps/lwipserver2/lwipserver2_x86_64.camkes

+#include <echo/tuning_params.h>
+
+component LWIPServer {
+    include "lwipserver.h";


What is the purpose of this include? It causes a lot of compiler errors and removing it seems to fix them.

kent-mcleod · 2022-03-25T06:22:07Z

I was able to successfully test this with imx8mm, but not able to yet with x86_64. Are there any special configuration options required for x86_64?

Signed-off-by: Lucy <[email protected]>

lucypa · 2022-04-04T02:10:38Z

@kent-mcleod Yes sorry i was missing one patch. It should work now with normal configurations with KernelExportPMCUser on.

axel-h · 2022-04-04T02:14:11Z

apps/lwipserver2/CMakeLists.txt

    ${libs}
    LD_FLAGS
-    -Wl,--section-start=.note.gnu.build-id=0x400920
+    -Wl,--section-start=.note.gnu.build-id=0x400920 #Ensure reproducible build


Does 0x400920 have an meaning actually or could this be simply "1" also?

lucypa added 3 commits March 18, 2022 12:07

Echo server application using lwip across 2 components

6fa1de4

Signed-off-by: Lucy <[email protected]>

Clean up

6db8a3e

Signed-off-by: Lucy <[email protected]>

License headers

5b33ae2

Signed-off-by: Lucy <[email protected]>

axel-h reviewed Mar 18, 2022

View reviewed changes

apps/lwipserver2/src/tcp_echo_socket.c

static ps_io_ops_t *ops;

static int peer_id;

Copy link

Member

axel-h Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsigned int?

kent-mcleod reviewed Mar 25, 2022

View reviewed changes

Fix support for x86

68b8225

Signed-off-by: Lucy <[email protected]>

lucypa force-pushed the lwip_echoserver branch from 20c75d4 to 68b8225 Compare April 4, 2022 02:10

axel-h reviewed Apr 4, 2022

View reviewed changes

-if("${KernelArch}" STREQUAL "x86")
+if(KernelArchARM)
+...
+elseif(KernelArchX86)
+...

		#define DMA_ALLOC_SIZE (DMA_RING_ALLOC_SIZE + BUF_SIZE * (TX_BUFS + RX_BUFS))


		/* Heap size */

Conversation

lucypa commented Mar 18, 2022 • edited by axel-h Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axel-h commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axel-h Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kent-mcleod commented Mar 25, 2022

Uh oh!

lucypa commented Apr 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lucypa commented Mar 18, 2022 •

edited by axel-h

Loading

axel-h commented Mar 18, 2022 •

edited

Loading

axel-h Mar 18, 2022 •

edited

Loading