Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No support for segmented odp packet in tcp_output & ofp_sosend_dgram #284

Open
HsuJv opened this issue Feb 28, 2023 · 3 comments
Open

No support for segmented odp packet in tcp_output & ofp_sosend_dgram #284

HsuJv opened this issue Feb 28, 2023 · 3 comments

Comments

@HsuJv
Copy link

HsuJv commented Feb 28, 2023

Hi there

I know that we have set the Interface MTU to 1500 at

#define OFP_MTU_SIZE 1500

which is smaller than the packet size(1856) set at
#define SHM_PKT_POOL_BUFFER_SIZE 1856

But in my use case, I'd like to make the MTU configurable to meet the requirements of jumbo frames (both TCP & UDP).

If I increase my MTU to some value that is greater than the packet size, it will result in a segmented odp_packet_t.
But the memcpy here in ofp_tcp_output.c

ofp/src/ofp_tcp_output.c

Lines 870 to 871 in f6580bb

ofp_sockbuf_copy_out(&so->so_snd, off, len,
(char *)odp_packet_data(m) + hdrlen);

and the memcpy here in ofp_uipc_socket.c
memcpy(p, data, resid);

do not check the segmented packets at all, which will cause a buffer overflow.

A simple approach to duplicate this issue is to reduce the SHM_PKT_POOL_BUFFER_SIZE to 1024 or smaller. Or calling ofp_sendto on the UDP sockets with len set to 2000 or greater.

Are there any plans to support this feature?

BRs.

@bogdanPricope
Copy link
Contributor

Hi @HsuJv,

To support jumbo frames you will need to:

  1. re-write ofp_mtu_set() to actually set the the MTU on the interface (odp_pktio_maxlen_set() ??) considering max capabilities (capa.maxlen.max_output, capa.maxlen.max_input) etc. Can be tricky .. if I remember correctly, ODP is considering ETH header in this frame size.
  2. Set a pkt_pool.buffer_size large enough for your use-case
  3. Fix the two memory copies... Good find!!

My code is different .. cannot say right now if this is enough.
I don't have plans to contribute to the project (I have my own version) but maybe the other members will. Else, it does not seems too complicated.

BRs

@bogdanPricope
Copy link
Contributor

Btw, pkt segments are not well supported in the entire stack... you should try to keep it simple with large enough packets and first segment size = pkt size...

@HsuJv
Copy link
Author

HsuJv commented Mar 1, 2023

Hi @bogdanPricope

Thanks for the quick reply.

For your answer:

  1. Yes, I've already hooked the ofp_mtu_set with a function pointer to export it to my application so that I can change it dynamic ly.
  2. Actually it is not a feasible approach to increase buffer_size as it will consume a lot more memory for connections that run small packets and will decrease the capability of traffic offload of my application sharply.
  3. Yes the memory copies are the root cause. I've not walked through the whole packet paths but I found that the ofp_cksum in ofp_in_cksum.c did do the segment check. So I assume that at least some of the developer has taken the segmented packets into consideration.

So in a short, fixing the issues with segmented packets seems necessary for memory saving and may not be very complicated work. It seems that this project has been active again recently so I submitted issues here to let the contributors know.

BRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants