[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [lwip-devel] in-place overwriting of payload via static "tcphdr" poi

From: Bernhart Pelger
Subject: Re: [lwip-devel] in-place overwriting of payload via static "tcphdr" pointer.
Date: Mon, 8 Jan 2018 15:21:25 +0100
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0

Am 08.01.2018 um 11:44 schrieb Simon Goldschmidt:
Bernhart Pelger wrote:
At first I also suspected the Xilinx driver to be at fault. But
after tracing the problem I think > that Xilinx can't really correct
the problem without a significant performance impact.

I mean, what is the Xilinx port driver (or any other driver for that
matter) supposed to do with received packets from the EMACs DMA ring
What about invalidating the dcache for buffers when passing the buffers
to the RX DMA ring *and* when getting the received packet from the RX
DMA ring?
This is was linux does, so why wouldn't that work for lwIP?

The Xilinx driver does not use custom_pbufs (as suggested in ZeroCopyRx.c).
Therefore it cannot know when a pbuf is not needed any more.
The Xilinx driver immediately frees the DMA buffer at RX interrupt (yes, before LWIP has processed it).

I know this sounds fundamentally broken, because other arriving packets might be overwriting the free'd DMA buffers before LWIP or the application had a chance to process it.

However, with 256 DMA buffers there is some time for 256 packets to arrive before a wraparound-overwrite occurs. And, If you permanently process packets slower than they arrive then you will lose packets anyways - at one place or the other.

There is no real reason for writing into received packets anyways.
You could byte-flip the TCP header fields on-the-fly when needed.
Please note that  LWIP's  UDP stack does not modify RX packets -- so
actually it *is* possible to leave rx packets unmodified.
You're right that we could always flip the bytes after reading. However,
in contrast to UDP, this is done multiple times for TCP. For example in
the OOSEQ code, this can be delayed.
What do you mean by "this (byte-flipping?) can be delayed"?
Do you mean "this (byte-flipping) can cause a delay"?

Sure, byte-flipped  port numbers and flags are needed in more places with TCP than with UDP.
But we are talking about 5-10 more occurances.
The performance impact from a few more ntohl() byte-flipps is really negligible.

Especially if you consider the otherwise required additional cache invalidate in the pbuf_free_custom() that invalidates again the same DMA buffer, which was already invalidated before the packet was handed to LWIP. Cache invalidation (especially for the full packet, not just TCP headers) costs a lot more time than that on-the-fly byte-flipping.

How would you ensure no application writes into the application part of
an rx pbuf?
- either by documenting that you are not allowed to modify rx pbufs.
- or by handing out 'const' pointers, (and documenting that you shouldn't
cast away the 'const').
That's not an option. If an application would need the received bytes but a
little different, it would have to do another memcpy.
Ok, I see that. There really may be applications relying on a modifiable rx pbuf. Then how about leaving everything as is, but still not modifying tcp headers in LWIP. Then the application can decide to not write to the pbuf if it has a driver that does not support that.

> I want to leave this open to the people actually implementing things.
Ok, leaving the application programmer more freedom is fine.
But what about the freedom for the driver programmer, to use single cache-invalidates for some performance gain?

Actually I tried to fix this issue by making all pointers to rx packet buffers
'const' in tcp_in.c, but I resigned after realizing the amount of changes that
would require.
I might be open to accept that change if it wouldn't hurt performance...
See above: Modification could still be allowed but at the applications responsibility. Then the driver provider would have to communicate that it does or does not supprt rx buffer modification. With no cache at all (or cache-coherent DMA) you could still modify your rx pbufs.

My current solution is to just patch lwip's tcp_input function and place a
cache flush for the TCP header at each exit point.
Move this invalidation to the point where the pbuf is inserted into the RX ring
and you should be fine.
That's directly in the RX ISR before the pbuf is prepared for LWIP.
There's already a cache invalidation there. Double-invalidating there would have no effect.

See this document Dirk adjusted some time ago:
Thanks. I didn't know there is support for a custom pbuf free function. Unfortunatley xilinx
doesn't use custom pbufs. I would have to rewrite that.

But that would still leave me invalidating the same DMA buffer two times: after packet reception and
when the pbuf is freed (after lwip and application processing).
As such, you probably should file a bug report with the xilinx port, not here...
I tried that two years ago, they don't react. Even If they did, the best 
would be to just copy the packets before handing them to lwip.
That doesn't surprise me. Vendor's drivers are often not very good ;-)


lwip-devel mailing list

Regards, Bernhart.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]