[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: Simon Goldschmidt
Subject: Re: [lwip-devel] in-place overwriting of payload via static "tcphdr" pointer.
Date: Mon, 8 Jan 2018 11:44:24 +0100

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
> buffer?

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?

> [..]
> 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.

> Honestly, I do not fully understand how this is not a problem for every
> lwip port driver, not just Xilinx.
> Many systems issue an RX interrupt and use (non-cache-coherent) DMA to
> transfer data from EMAC to memory.
> Since memory is not unlimited, new rx packets will at some point overwrite
> old rx packets (ring buffer).

It is, if the driver is not well programmed. Either you run on a processor
without dcache or you have to understand when you have to invalidate the cache.
Sadly, there are just not too many drivers out there supporting zero copy rx.

As response to your post:

>> 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.

>> Remember that rx pbufs are not 'const' for lwIP.
> But they should be const.

No, they shouldn't. See above.

> Normally an application does not need to write into the received packets.
> I can't think of a good reason for that,

Just because you can't doesn't mean noone can ;-)
I want to leave this open to the people actually implementing things.

> especially not for TCP, where you normally have to assemble/copy the data
> stream from multiple pbuf payloads anyways! If you need to patch rx data at
> all, then why not patch the assembled stream data?

A well written zero copy application does not need to assemble or copy but can
hand the received pbufs as scatter/gather to the next driver.

> Patching rx data is also not good programming practice: multiple entities
> (EMAC and application/lwip) would write to the same memory.
> This makes code less readable and can lead to race conditions.

Nooooooo! There are precise points who is in control of RX pbufs. First the
DMA, then lwIP, then the application. No race conditions there!

> 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...

> 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.

See this document Dirk adjusted some time ago:

> 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 
> solution
> 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 ;-)


reply via email to

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