STM32 MAC driver optimization

[iggarpe]

Thinking in OOP that function would be a method of the descriptor not of the driver, if it cannot be avoided then I would add a member to the descriptor.

Yes, with the added benefit of not changing the existing API. Also, if both MACDriver and MACReceiveDescriptor had to be passed to mac_lld_release_receive_descriptor, the lwipthread layer would still have to encapsulate both of them together and tie to the pbuf chain.

Though conceptually a call to mac_lld_release_receive_descriptor doesn't make sense outside of a MAC driver context.

This is purely a conceptual construct, you can see as well the descriptor as the object that encapsulates everything related to a frame, including buffers.

Well, I was thinking actually more along the lines of wasted memory. Just a few bytes per frame, so I'm probably being paranoid because I'm having trouble with the available SRAM in my application.

Agreed, this pool can be implemented in lwupthread, there is no need for the driver to handle the pool so it does not impact us in this moment.

I though of a way to not have to use a separate pool for those structs, but I'm sure you won't like it (neither I do much): use a #define to make mac_lld reserve some padding space at the beginning of each buffer. Data would beging after this padding. Then lwipthread would place the custom pbuf structure there. An #if in lwipthread would be needed to make sure the padding space is at least as large as the custom pbuf struct.

I agree, we need a mechanism to associate a callback only to the last element of a pbuf chain and release the descriptor there.

That is not a problem: set the PBUF_FLAG_IS_CUSTOM only for the last pbuf in the chain. The callback will be called by pbuf_free only for that particular pbuf. In fact, you could also use the first pbuf, doesn't really matter.

Handling buffers one by one is not really necessary, it is just how lwIP does things.

Yep, but to be totally honest, I'm with jcbarlow here. lwIP may not be perfect, but works quite well, is very mature and writing a new TCP/IP stack from scratch is an enormous task, so making Chibi/OS [b]just a bit[/b] dependant on how lwIP handles buffers is not such a bad thing.

And most importantly, I'll insist that I don't see a conceptual problem with the individual buffers becoming independent objects once they are read with mac_lld_get_next_receive_buffer. There can be just a mac_lld_release_receive_buffer function taking as argument ONLY the buffer address. Any other information that mac_lld driver needs to properly dispose of or recycle this memory can be stored in a special, internal header located before the buffer and that is easily accessible by just substracting an offset.

I'm definitely going to implement zero-copy with this approach and see how it goes, then once it's complete and tested you can evaluate whether merge it into trunk as is. Remember: it imposes no restrictions on the existing drivers: they can remain as they are now as long as they stick to non-zero-copy.

I think you should finish it and then offer it as an alternative implementation, this is why I think it should be a drop-in replacement for the current implementation. If somebody want the functionality it must be as simple as replacing the files, everything else should be transparent.

So far I think it would require to replace mac_lld.c, mac_lld.h and lwipthread.c. It will require also IP_FRAG enabled in lwipopts.h (though with a tiny patch to lwIP we could allow enabling the custom pbufs in lwipopts.h without having to enable IP_FRAG too).

[iggarpe]

Giovanni,

Please see patch below: restarts the DMA engine when a descriptor is purged (due to frame errors) in mac_lld_get_receive_descriptor.

Code: Select all

Index: mac_lld.c
===================================================================
--- mac_lld.c   (revisión: 5041)
+++ mac_lld.c   (copia de trabajo)
@@ -504,6 +504,14 @@
     }
     /* Invalid frame found, purging.*/
     rdes->rdes0 = STM32_RDES0_OWN;
+
+    /* If the DMA engine is stalled then a restart request is issued.*/
+    if ((ETH->DMASR & ETH_DMASR_RPS) == ETH_DMASR_RPS_Suspended) {
+      ETH->DMASR   = ETH_DMASR_RBUS;
+      ETH->DMARPDR = ETH_DMASR_RBUS; /* Any value is OK.*/
+    }
+    
+    /* Proceed to next descriptor.*/
     rdes = (stm32_eth_rx_descriptor_t *)rdes->rdes3;
   }

[iggarpe]

Giovanni,

A couple of issues:

1- Typo in os/hal/src/mac.c: magGetNextReceiveBuffer

2- Wouldn't it be useful to be able to define the link status polling interval in lwipthread.c ?. Currently it's fixed to 5s in lwipthread.c and I lowered it to 1s.

[Giovanni]

Hi,

Committed both.

Giovanni

[iggarpe]

Giovanni,

I got it working. Both zero-copy and multibuffer (reception). Performance increase in my scenario is dramatic, I can now handle about 8 times as many packets in a burst with the same SRAM usage, due to (a) the multibuffer feature that allows better use of the low level driver buffer memory when frames are small and (b) now I can set a lower value of PBUF_POOL_SIZE and use that memory instead for low level driver buffers. Amazing.

I would like to perform more extensive testing (and maybe implement zero-copy transmission) before hading you the code for revision. Meanwhile I would like to have your opinion on a few implementation details:

1- I'm using a mac_lld_release_receive_buffer function. I know you don't like the idea much but I can assure you it allows, by much, for the simplest implementation. Remember that for drivers in which it might not be possible to implement multibuffer (i.e. several buffers per frame), they can choose to return only a single buffer in mac_lld_get_next_receive_buffer.

2- mac_lld_release_receive_descriptor is there to stay, both in non-zero-copy and zero-copy modes. In non-zero-copy mode it releases all the buffers of a MACReceiveDescriptor, in zero-copy mode it releases only those buffers that were not "claimed" with mac_ldd_get_next_receive_buffer. This is very convenient for lwipthread, because in the multibuffer case you may encounter a allocation error for the pbuf chain you're building, and when that happens you just stop calling mac_lld_get_next_receive_descriptor, free the pbuf (the deallocation callback will release the buffers already "claimed" and placed into those pbufs), and finally call macReleaseReceiveDescriptor that will release the buffers that were not "claimed".

Now, I realize the mac_lld_release_receive_descriptor / mac_lld_release_receive_buffer duality is there just because of the multibuffer feature. I know you're not very fond of it, but I'm telling you that feature cannot be implemented without those two functions in the API, so I would kindly ask you to accept that change into the trunk. Drivers that do not implement zero-copy need not worry about it. Drivers that implement zero-copy but not multibuffer can just see mac_lld_release_receive_buffer as a special function to be used from the pbuf deallocation callback. They are always returning one single buffer, so there is a MACReceiveDescriptor <---> buffer identity and no atomicity or other exotic problems.

3- mac_lld_release_receive_buffer takes one single argument: the pointer to the data buffer. I'm not sure I like this (and forced me to use a trick, see below), I'd rather pass also a MACDriver pointer. I'd rather pass it too in mac_lld_release_receive_descriptor. Conceptually I think it makes sense that all functions in mac_lld require a driver context (except init/start).

The "trick" I mentioned is simple:

Code: Select all

typedef struct {
  MACDriver *macp;
  uint32_t data[RX_BUFFER_SIZE];
} stm32_eth_rx_buffer_t;

Then from a pointer to the 'data' member you can easily obtain a pointer to the whole structure and read the MACDriver pointer.

I would prefer instead to have a MACDriver argument passed to mac_lld_release_receive_buffer. And in that case also a MACDriver argumento would be needed for mac_lld_release_receive_buffer (which calls the release buffer function, now doesn't need the MACDriver function just because the release buffer function doesn't, thanks to the "trick").

4- You'll be glad to hear that modifications to lwipthread are minimal, simple, and easy to read.

[iggarpe]

Oops, it seems I called victory too soon. All my services (including an FTP server) worked as long as I kept a huge log of low level driver buffers (wanted to test limits).

As soon as I reduced the amount of buffers to a more reasonable number, TCP and ICMP stopped working. From what I have read some code reuses the incoming pbufs to send a reply (at least ICMP does for sure, I suppose TCP does too). So my UDP services work fine but TCP and ICMP are broken.

So, in light of this new information, looks like zero copy reception is just NOT POSSIBLE in lwIP as is (i.e. using pbuf_custom functionality).

There's an alternative implementation in which lwipthread continuously allocates pbufs and feeds them to the low level driver to be used by DMA for reception, but I suppose this breaks the driver model in so many levels that you don't even want to hear about it. Just think of an architecture where buffers need to be in a certain address space...

So I'm putting zero-copy on hold for now, remove all zero-copy related code and submit a patch for the multibuffer feature only.

[iggarpe]

Update:

Confirmed that lwIP is making assumptions on where the received pbufs come from (PBUF_POOL) and reusing them, so it is just not possible to implement zero-copy reception in the "proper" way (i.e. buffers "owned" by mac_lld). Fixing lwIP is out of the question.

I definitely need zero-copy, not for performance reasons but to be able to cope with large incoming packet bursts. Unless buffers are "shared" by lwIP and mac_lld, I need to allocate a lot of space in both domains. I don't have that much SRAM. zero-copy has the side effect of allowing a single pbuf pool for reception.

Since the "proper" way is not possible I'm implementing this the other way around: mac_lld will be initially GIVEN buffers to store received data into, it will return those buffers to lwipthread which will build a pbuf chain and pass it up to lwIP guts. The buffers passed to mac_lld will come from PBUF_POOL, and lwIP pbuf_free function will be patched to feed them back to mac_lld for reuse.

The "buffer" object must be opaque to mac_lld (I don't want to make the unaceptable mess of having mac_lld to know about pbufs), so each buffer object will be comprised of two elements: a data pointer and a void pointer. The second will be used to store the pbuf pointer when the buffer object is passed to mac_lld and to get the pbuf pointer when the buffer object is returned by mac_lld_get_next_receive_buffer.

I think this is the cleaner way to do this. It respects the driver model (mac_lld doesn't deal with pbufs) and makes minimal changes to lwIP.

Also, it has a GREAT side effect: since you are allocating the reception buffers outside of mac_lld, you can choose to do it from a pool placed in a specific memory area. The STM32F4 has a 16KB SRAM block to which DMA can access with not contention with respect to the other 112KB SRAM block.

Let's see how this ends up.

[Giovanni]

I've found this one too that looks interesting http://fnet.sourceforge.net/ at very least it has a nice web side and documentation.

[rubenswerk]

Hello,

I just found this topic and saw that there are some patches increasing the MAC performance - very nice

One hint to iggarpe:
I also did some testing with massive ping flooding (no pause between packets, maximum packet size). I found out that this situation leads to a memory leak in lwip.

viewtopic.php?f=3&t=23&start=180#p3888

Maybe this is relevant for you, because you're talking about packet bursts in your use case.

Bye, RuWe

[iggarpe]

Giovanni,

I got it working and would like to tidy it up so it has at least some odds of getting into trunk someday. Need your input.

Long story short: lwIP requires for input pbufs that the data buffer follows the pbuf header. It makes some assumptions that won't work otherwise. The struct pbuf and the buffer MUST be consecutive. Fixing this in lwIP would require a lot of changes and testing so it is off the table. To make things worse lwIP configurable size and memory alignments make it quite difficult to get a pointer to the pbuf from a pointer to the buffer, that is, a simple substraction will not work in all cases (I mean in different scenarios of compiler alignment and lwIP configure data and memory alignment).

So given these constraints there are two approaches to implementing zero-copy reception:

a) The reception buffers belong to lwipthread and are handed to the low level driver.

b) The reception buffers belong to the MAC low level driver, but we can somehow force it to not use a fixed portion at the beginning, which will be used by lwipthread to build the pbuf header.

Besides the above, lwIP requires some minimal patching. We need to hook the pbuf deallocation to give the buffer back to the MAC low level driver. The existing pbuf_custom mechanism is NOT USABLE (too long to explain why), so I've created a new pbuf of type PBUF_MAC which is handled exactly like a PBUF_RAM/PBUF_POOL except a special external function is called by pbuf_free to deallocate it. lwipthread uses this callback to give the buffer back to the MAC low level driver so it can be reused for reception.

Give the constranints lwIP imposes (I hate it but it's what we have as of now), I don't think there is a "clean" aproach.



With (a), you can go with a simple zero-copy API for the MAC low level driver:

Code: Select all

uint8_t *mac_lld_get_next_receive_buffer(MACReceiveDescriptor *rdp, size_t *sizep, void *token);
void mac_lld_set_receive_buffer(MACDriver *macp, uint8_t *data, size_t len, void **token);

The token is an opaque argument that is taken by the low level driver and internally paired to the passed buffer, so they are both returned by mac_lld_get_next_receive_buffer. In practice the token is the pointer to the pbuf structure, whose location is not trivial to calculate despite being in front of the buffer (shame on lwIP).

Note that during initialization you need to call mac_lld_set_receive_buffer multiple times so the MAC low level driver starts with some usable buffers. From there on they are returned by mac_lld_get_next_receive_buffer, passed up to lwIP and returned to the MAC low level driver by the special deallocation callback (using mac_lld_set_receive_buffer again).

In this case the buffers are instantiated in lwipthread, which I don't like at all.



With (b), we would need a new function like this:

Code: Select all

void mac_lld_set_receive_buffer_front_padding(uint32_t n);

To be called before mac_lld_start and which would guarantee that AT LEAST the requested space is reserved and usable IN FRONT of the returned buffers. An argument in mac_lld_init or mac_lld_start would do the job too.

In this case the buffers are instantiated in the MAC low level driver (where they should be), and there is no need to call mac_lld_set_receive_buffers before mac_lld_start.



Which do you think is the less ugly approach ?