Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Official NIC/DPDK component support to Composite #451

Merged
merged 74 commits into from
Jun 20, 2022
Merged

Conversation

betahxy
Copy link
Contributor

@betahxy betahxy commented May 2, 2022

Summary of this Pull Request (PR)

Add description here.

Intent for your PR

Choose one (Mandatory):

  • This PR is for a code-review and is intended to get feedback, but not to be pulled yet.
  • This PR is mature, and ready to be integrated into the repo.

Reviewers (Mandatory):

(Specify @<github.com username(s)> of the reviewers. Ex: @user1, @user2)

Code Quality

As part of this pull request, I've considered the following:

Style:

  • Comments adhere to the Style Guide (SG)
  • Spacing adhere's to the SG
  • Naming adhere's to the SG
  • All other aspects of the SG are adhered to, or exceptions are justified in this pull request
  • I have run the auto formatter on my code before submitting this PR (see doc/auto_formatter.md for instructions)

Code Craftsmanship:

  • I've made an attempt to remove all redundant code
  • I've considered ways in which my changes might impact existing code, and cleaned it up
  • I've formatted the code in an effort to make it easier to read (proper error handling, function use, etc...)
  • I've commented appropriately where code is tricky
  • I agree that there is no "throw-away" code, and that code in this PR is of high quality

Testing

I've tested the code using the following test programs (provide list here):

  • micro_booter
  • unit_pingpong
  • unit_schedtests
  • ...(add others here)

WenyuanShao and others added 30 commits January 19, 2022 00:23
- linker script should be ARCH related
- make necessary changes so that DPDK can successfully compile drivers
- some libs need this capability to print in Composite without modifying much code
- successfully probed 1000 pmd and read netcard information like mac address
- set stack size to be 4*4k, all_stack_size to be 66560*4
- this is because some applications like DPDK would require a larger stack size, or the stack could be corrupted
@betahxy betahxy requested a review from gparmer May 2, 2022 05:09
Copy link
Collaborator

@gparmer gparmer left a comment

Choose a reason for hiding this comment

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

Went through the bench/test and lwip. Looks good to go to me! @WenyuanShao ?

I'm approving, but get @WenyuanShao 's opinion, and I suggested some small fixes.

```shell
sudo apt install -y python3-scapy netsniff-ng
```
`scapy` is a python-based packect manipulation tool. You can use it to generate any king of packets you want. `netsniff-ng` is a network analyzer tool set. We will only use its `trafgen` to generate traffic and `ifpps` to watch the traffic's speed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

king -> kind

Use a spell checker in your editor to help out with this!

```shell
sudo trafgen --cpp --out tap0 --conf ./src/components/implementation/tests/bench_dpdk/traffic_template.trafgen --verbose --cpu 1 -b 1000MiB
```
4. Start the program
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Start the program" -> "Start the composite networking system"

Given your directions here, I assume it is OK to start composite after you're generating the workload with trafgen. I would have assumed you wanted to start the workload after composite boots up. But if this works, that's great!

sudo ./cos run bench_dpdk_test enable-nic
```
5. Watch the `ifpps` monitor
Now you will see in the terminal the rx/tx speed of the `tap0` device. Then you can use `Ctrl+C` to stop the `trafgen`. (At this pointed, If the `trafgen` cannot be stoped, you need to start again the dpdk program and then stop it to make sure the `trafgen` is stopped.) Finally, use `Ctrl+C` to stop the `ifpps`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"The dpdk program" Don't you mean "start again the composite dpdk system"

@@ -0,0 +1,53 @@
## How to perform this DPDK test?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the scripts and config files in this directory, don't these commands need to be run in this directory? If so, say so.

#define PACK_STRUCT_END

#include <llprint.h>
#define LWIP_PLATFORM_DIAG(x) do { printc x; } while (0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used, and it compiles? I'm unaware of the printc x syntax, but I'm guessing that you're assuming that x includes (...). If that's the assumption, might be good to state it, and why this might be used.

#ifdef NIL
#define PERF_START { \
unsigned long __c1l, __c1h, __c2l, __c2h; \
__asm__(".byte 0x0f, 0x31" : "=a" (__c1l), "=d" (__c1h))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is all of this? Can we document it? Get rid of it? etc....


#define MEMP_NUM_TCP_PCB 512 /* need a fair amount of these due to timed wait on close */
#define MEMP_NUM_TCP_PCB_LISTEN 128
#define LWIP_ARP 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be useful on the actual hardware. @WenyuanShao will have to figure out if we need this there.

@WenyuanShao
Copy link
Contributor

@gparmer no further comments. Looks good to me as well. I'll start thinking about testing it on the hardware we have since you approve the PR. @betahxy, really exciting having DPDK officially in Composite.

@betahxy
Copy link
Contributor Author

betahxy commented Jun 19, 2022

@gparmer I've fixed the suggestions above. Thank you so much for the review!

@gparmer gparmer merged commit b42e20c into gwsystems:main Jun 20, 2022
@gparmer
Copy link
Collaborator

gparmer commented Jun 20, 2022

Thanks for the great research on this @betahxy !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants