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

[Bernhart Pelger]

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

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:
> http://git.savannah.gnu.org/cgit/lwip.git/tree/doc/ZeroCopyRx.c
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 
> > 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 ;-)
>
> Simon
>
> _______________________________________________
> lwip-devel mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/lwip-devel

Regards, Bernhart.