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

EMAC memory manager improvements & fixes #426

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

multiplemonomials
Copy link
Collaborator

@multiplemonomials multiplemonomials commented Jan 22, 2025

Summary of changes

This PR makes a number of fixes to the EMAC memory manager interface and to the three implementations of the manager (LWIP, Nanostack, and the EMAC test's mem manager). These are required for my upcoming STM32 EMAC MR.

Main changes are:

  • Adding a "get_lifetime()" function which can be used to tell what type of buffer a buffer is and how long it will live. This is important so that EMAC drivers can know whether the buffer needs to be copied!
  • Making network stack independent options for the Rx buffer pool size and buffer size. This is needed so that the Nanostack memory manager can (pretend to) have a buffer pool that the EMAC can use.
  • Adding a callback from the memory manager to the EMAC for when pool buffers are freed. This is important so that the EMAC can reallocate Rx descriptors after new memory becomes free.
  • Updating EMACTestMemoryManager to support the concept of an Rx buffer pool with limited size, so that EMAC drivers can be tested correctly
  • Changing greentea tests to not select any network stack by default (needed for EMAC test). Now we can compile all the netsocket tests with both LwIP and nanostack!

Impact of changes

Memory manager should have some new functions and be better documented.

Migration actions required

Documentation


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR


uint8_t allocSrc = pbuf_get_allocsrc(p);

if (allocSrc == PBUF_TYPE_ALLOC_SRC_MASK_STD_MEMP_PBUF && PBUF_NEEDS_COPY(p)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See this enum for details on this

* Memory buffers can be allocated either from heap or from memory pools. Heap buffers are always contiguous.
* Memory pool buffers may be either contiguous or chained depending on allocation size.
* Memory pool buffers may be either contiguous or chained depending on allocation size. By LwIP convention,
* the pool should only be used for Rx packets -- the EMAC may opt to keep buffers pre-allocated from the pool
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's just say I learned this one on the job, the hard way

@@ -304,7 +304,7 @@ static int8_t mbed_trace_skip(int8_t dlevel, const char *grp)
if (m_trace.filters_include[0] != '\0' &&
strstr(m_trace.filters_include, grp) == 0) {
//grp was in include list
return 1;
return 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This looks like a bug, I'm 99.9% sure. It was causing the include list to be treated as an exclude list! Of course, the include list is still kinda pointless, because it seems to do the same thing as just not putting the buffer on the exclude list, but this makes things slightly less messed up.

@@ -189,34 +181,19 @@
"RZ_A1XX": {
"tcpip-thread-stacksize": 1328,
"default-thread-stacksize": 640,
"memp-num-tcp-seg": 32,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having individual targets change the TCP settings is causing some build issues (because these LwIP thinks these targets would also need more Rx pool size). I don't know if it's correct or not but either way it's really weird for rando targets to use different TCP settings. If the default TCP settings are wrong for some applications then we should change them everywhere, or provide a guide for users to change them.

@multiplemonomials
Copy link
Collaborator Author

Unfortunately this PR seems to have unearthed some sort of segfault in the LPC1768 LwIP TCP test
image

It doesn't seem directly related to the test -- looks more like some kind of memory corruption vulnerability. I don't think I have the time to debug this right now but it should def be looked into, esp. if it persists after refactoring the LPC EMAC

@multiplemonomials multiplemonomials force-pushed the dev/emac-mem-manager-improvements branch from 6b06a72 to 2cf8902 Compare January 26, 2025 22:05
@multiplemonomials
Copy link
Collaborator Author

Actually... it appears that LPC1768 ethernet with LwIP is basically broken by this PR. I spent a good hour looking around but I wasn't able to find a smoking gun. I really don't see what exactly changed, but I think the EMAC driver was a bit shaky before (wasn't passing all the EMAC tests), and it has totally broken down now. Nanostack, somehow, does still work, but LwIP and the EMAC test suite seem to be unable to receive packets.

I am planning to totally rewrite the LPC1768 EMAC driver in the near term anyway, so I think I'm going to table this one until I can do that rewrite. It just isn't worth wasting all this effort on a driver whose days are numbered anyway.

On the bright side, I tested this branch with NUCLEO_F429ZI and K64F and all network tests passed there!

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.

1 participant