STM32 MAC driver optimization

[iggarpe]

Giovanni,

I've given this matter quite a lot of though and came to the following conclusions:

(I'm restricting myself to the receive path which is the one that most matters in this regard, we can try to transpose all this to the transmit path later)

1- The MACReceiveDescriptor semantic is wrong. It gives the caller the impression it's dealing with DMA descriptors, which may or may not be true. It is not true for drivers that do not have DMA but use FIFO. It is not true for multibuffer implementations. The "MACReceiveDescriptor" structure is actually, as you very well described it, an ACCESSOR. Let's call it MACReceiveAccessor instead. Induces less confussion.

2- The MACReceiveAccessor usage pattern is:

get --> read --> ... ---> read --> release

That is true for the "old" non zero-copy interface in which the entities being read from the accessor are raw bytes. This entity does not need require any special interaction beyond the point in which it is read using mac_lld_read_receive_descriptor.

And that is also true for the "new" zero-copy interface, in which the entities being read are buffers. But unlike the non zero-copy interface, this entity is not simple and has its own usage pattern:

get --> use --> release

A buffer is not an accessor. We cannot tie a buffer to an accessor. For one thing, accessors are intended to be allocated by the caller (most likely in the stack) and be short living, that is, be used to actually access the data and then released. In your zero-copy proposal the accessor would have to live longer and be passed upwards by lwipthread to lwIP code and then to the application layer. That means the accessor itself needs to be allocated from a heap or pool or something, making IMHO things more complicated than necessary.

I propose that we stick to the actual usage patterns, both for the accessor and the buffer, and as of now, with respect to your proposal, all that's needed is a mac_lld_release_receive_buffer function. That's all.

Conceptually, the user of the MAC API would be doing this for accessors:

1- Get accessor.
2- Read buffers from the accessor (and then encapsulate and send them upwards).
3- Release the accessor.

And for buffers:

1- Get a buffer (this actually happens in step 2 above).
2- Use the buffer (this happens at the application layer).
3- Release the buffer (also at the application layer).

Note that API allows implementation of both zero-copy and multibuffer frames.

Now for your concerns:

MAC periphreral doesn't have DMA ?

Don't use the zero-copy API.

MAC periphreral does have DMA but does not support multibuffer ?

Use the zero copy API and one buffer per frame (after all, it's the LLD driver who decides how many buffers per frame it allocates and later returns in successive calls to mac_lld_get_next_buffer).

MAC periphreal does have DMA and support multibuffer but there are atomicity issues with separate release of buffers ?

Same, use the zero copy API and one buffer per frame.



Then, for STM32 MAC periphreal that will gladly handle indepentent buffer releasing, I can implement both zero copy and multibuffer frames for efficient use of receive memory in small frames.

What do you think?.

[Giovanni]

Hi,

I will look into this during weekend, I am willing to give it a try.

Giovanni

[iggarpe]

I'm also working on a tentative implementation, and have a comment:

mac_lld_release_receive_descriptor is problematic.

I just realized that in the zero copy case it should NOT RELEASE the DMA descriptors/buffers (i.e. set the OWN bit). So either we use a #if MAC_USE_ZERO_COPY inside its code to make it behave differently (!!!) when MAC_USE_ZERO_COPY is enabled or we remove it altogether if MAC_USE_ZERO_COPY is enabled.

Alternatively, some scheme could be used to "mark" buffers when they are read with mac_lld_get_next_receive_buffer, so the associated descriptor is not released to the DMA engine when mac_lld_release_receive_descriptor is called. I'm thinking of something like setting the DMA descriptor to NULL after it's been read, and in mac_lld_release_receive_descriptor check for the pointer and not release it if it's NULL.

[iggarpe]

QUESTION: is it harmful in any way to pass to lwIP input the whole received ethernet frame including the 4 byte CRC at the end ?

I'm asking because it makes multibuffer handling code more complicated (need to account for the case where the 4 byte CRC lies across the last two buffers). If, as I suspect, the only harm is use of 4 extra bytes of storage, I'd rather use them and keep the code simple and clean.

[Giovanni]

I think that multi buffers can be implemented without adding new functions: if you still want to release buffers one by one (OWN bit), this can be done without having a specific function, just release a buffer when the next one is retrieved, release the last one when the descriptor is released.

For example, if a descriptor links 2 buffers, it requires two calls to "get next buffer° and a final call to "release descriptor".

1) wait for descriptor
2) get next buffer (return #1, nothing released).
3) get next buffer (return #2, release #1).
4) release descriptor (release #2 and release the descriptor itself).

Do you see problems with the this approach? Less functions involved.

In general I am opposed to bringing at API level things not strictly required by the application layers. Designing an API around a specific low level implementation is not a good idea.

Not sure about the question regarding CRC, we should ask to somebody more inside lwIP.

Giovanni

[Giovanni]

Thinking again the sequence would more be like:

1) wait for descriptor
2) get next buffer (return #1, nothing released).
3) get next buffer (return #2, release #1).
3) get next buffer (return NULL, release #2).
4) release descriptor (release the descriptor itself).

No need to release buffers from the "release descriptor" context.

Giovanni

[iggarpe]

Mmm... I think I'm probably not making myself clear enough.

The buffers CANNOT be released as the accessor is iterated. They will be released later at the application layer when pbuf_free is used. You cannot set the OWN bit of a descriptor because it no longer has an associated buffer (well, it has, but is being used).

One clarification that I should have made: in my approach buffers are no longer tied to fixed descriptors. This is the sequence of events:

1- Initialization: all descriptors set with an associated buffer, all OWN bits set in descriptors.
2- A frame arrives, N descriptors are used to store it, OWN bit is cleared in those descriptors.
3- lwipthread gets an accessor to this chain of descriptors.
4- lwipthread gets a buffer from the chain by calling mac_lld_get_next_receive_buffer.
5- Repeat (4) until no more buffers left.
6- Build a pbuf chain with a deallocation callback and pass it up to lwIP.
7- The application layer gets the pbuf chain, does whatever it needs to do with it, and calls pbuf_free, which in turn calls the deallocation callback which calls mac_lld_release_receive_buffer.
8- mac_lld_release_receive_buffer places this buffer in the first descriptor that is not owned by DMA and then sets the OWN bit.

You cannot set the OWN bit in the descriptor until you have set a new buffer to that descriptor, and you can't do that because there are no buffers left (I'm assuming the same number of buffers and descriptors, see note (*) below).

Think of it as a pool of buffers. Upon initialization the pool is empty as all buffers are assigned to "ready" descriptors (ready = owned by DMA and ready to receive data). As a frame is received some of those descriptors become "busy" (busy = not owned by DMA, pending data processing). Then lwipthread reads the buffers and builds a pbuf chain which is passed upwards to lwIP. At this point if there were buffers in the pool you could just get some, assign them to the descriptors and make them "ready" again, but the pool is still empty. At some point in the future the application layer calls pbuf_free, which releases the buffers. This would add them to the pool, and from there you assign them to "busy" descriptors and make them "ready".

Of course, having a pool here doesn't make sense if you just have the same number of descriptors and buffers. By the time the buffers are indirectly released by a call to pbuf_free in the application layer you KNOW that there are enough "busy" descriptors waiting for free buffers, so as each buffer is released you just assign it to a "busy" buffer and make it "ready".

(*) NOTE: of course you could have, say, 10 descriptors and 40 buffers. That would mean that upon initialization you still have 30 free buffers and you could assign them to "busy" descriptors as their used buffers are read with mac_lld_get_next_receive_buffer, but that solves nothing. At some point you might still run out of buffers if they are not processed fast enough, and you still need lwipthread to make full use of the 40 buffers. On the other hand if you have 40 descriptors and 40 buffers the MAC DMA will handle by itself a burst of 40 frames without any intervention of lwipthread.

[iggarpe]

For what is worth, I just checked and lwIP works fine if you pass the whole ethernet frame with the 4 byte CRC at the end.

Makes sense. The tcpip_input description says it accepts a pointer to an ethernet header, which to me seems sort of imply that it cares only about the frame size it reads from the header. So any stuff beyond that point is simply ignored.

Would you agree trading those extra 4 bytes for code simplicity is worth it ?

[iggarpe]

I've completed a preliminary version. Haven't modified yet lwipthread to make use of the zero-copy API:

https://dl.dropbox.com/u/35035041/mac_l ... _r5032.zip

As the name implies, based on r5032. Changes:

Multibuffer (reception only):

- MACReceiveDescriptor now has pointers to first, last and current physical descriptors, plus physdesc_offset (offset into current physical descriptor).
- Split STM32_MAC_BUFFERS_SIZE to STM32_MAC_TRANSMIT_BUFFER_SIZE and STM32_MAC_RECEIVE_BUFFER_SIZE, so size of transmit and receive buffers can be specified independently.
- Rewritte mac_lld_get_receive_descriptor to handle multibuffer frames.
- Rewritte mac_lld_read_receive_descriptor to handle multibuffer frames.
- Rewritte mac_lld_release_receive_descriptor to handle multibuffer frames.
- Rewritte mac_lld_get_next_receive_buffer to handle multibuffer frames.

Zero copy (reception only):

- In MACDriver replaced rxptr by rxhead and rxtail.
- Added #if !MAC_USE_ZERO_COPY/#endif around code inside mac_lld_release_receive_descriptor so it does nothing when the zero copy is enabled.
- Added mac_lld_release_receive_buffer: adds the passed buffer to the first non-DMA-owned descriptor (starting at rxtail). See comments, it's quite well explained.

As I mentioned in a previous message, multibuffer functionality is fully tested and works like a charm. Zero copy untested because I'm yet to figure out how to build the pbuf chain myself in lwipthread. BTW: a small lwIP patch is definitely going to be needed: the pbuf deallocation callback mechanism that ip_frag uses is very limited (does not allow passing an argumento to the callback function, and we need to pass the MAC driver pointer) and a special pool is used because the custom pbufs are a bit bigger than standard ones (because of the pointer to the callback function). We do not want to use the IP_FRAG pool, so we need a separate one. Since each LLD buffer is becoming a pbuf in lwipthread, it would be VERY convenient to just reserve some space at the beginning of each buffer for a custom pbuf structure. Could be done in an opaque fashion, but I don't like this much. It's forcing something into mac_lld just to make things easier in lwipthread. What do you think ?.

(sh!t, just realized I forgot to change MAC???Descriptor to MAC???Accessor, ???_descriptor to ???_accessor, and so on, I still think this would be convenient, do you agree?)

[iggarpe]

The code above lacks an obvious optimization (that I'll implement this evening):

mac_lld_release_receive_buffer instead of just assigning the released buffer to the descriptor pointed by macp->rxtail, needs to search for a descriptor that is not already owned by DMA. This is because of the way descriptors and their associated buffers are purged when an error is detected: the OWN bit is just set. If instead of just setting the OWN bit of the descriptor that is being purged, I must reassign its buffer to the descriptor pointed to by macp->rxtail (which may or may not be the SAME decriptor being examined) and set its OWN bit. With this scheme macp->rxtail always points to a descriptor not owned by DMA that is waiting to be assigned a buffer.