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

"Fragmentation Prohibited": message/connection property? #226

Closed
britram opened this issue Sep 7, 2018 · 13 comments
Closed

"Fragmentation Prohibited": message/connection property? #226

britram opened this issue Sep 7, 2018 · 13 comments
Assignees

Comments

@britram
Copy link
Contributor

@britram britram commented Sep 7, 2018

Discussion on #203 identified that we probably want a Message (and/or Connection) property that forces DF on for IPv4 (and, I guess, ensures that datagram sends on IPv6 below MTU will fail, by disabling sender fragmentation, as well).

@gorryfair
Copy link
Contributor

@gorryfair gorryfair commented Sep 7, 2018

This should also prevent IPv4 sender fragmentation. It's possible to generate IPv4 fragments from the stack with the DF set.
I think the property should default to do this, and I'd suggest "ALLOW Network Fragementation" as a suggested name.

@philsbln
Copy link
Contributor

@philsbln philsbln commented Sep 7, 2018

This has to be a Message Property, however, we expect programmers to typically set this property on a Connection or Preconnection (as default for all messages).

@mwelzl
Copy link
Contributor

@mwelzl mwelzl commented Sep 7, 2018

@gorryfair so you say "network fragmentation". This excludes fragmentation in the host, which we may also want to control if we want to send unfragmented PMTU-sized packets. On the other hand, if this includes fragmentation in the host, your suggested default will always fail for TCP when I send messages > PMTU, yet in case of a TCP-like service I really don't want to care about this.

=> My suggestion would be to just to drop the word "network" and (alas) NOT make forbid-fragmentation the default.

@philsbln it has to be a Message Property, yes. But what makes you say that you'd expect programmers to typically set this on a Connection or a Preconnection?

@philsbln
Copy link
Contributor

@philsbln philsbln commented Sep 7, 2018

@mwelzl the only use case to set this on a per-message basis I can image is PMTU discovery and I hope this is usually done by the transport system implementaion

@mwelzl
Copy link
Contributor

@mwelzl mwelzl commented Sep 7, 2018

@philsbln I agree with you but don't see the relationship with setting that on a Connection or Preconnection.

@tfpauly
Copy link
Contributor

@tfpauly tfpauly commented Sep 7, 2018

Agreed to the general point that this is a useful property!

To the discussion of where it goes, I agree with @philsbln. We had discussed letting defaults for Messages be set connection-wide for convenience, so I think the point is that this would commonly be set as a convenient default, rather than having the application set it over and over.

@philsbln
Copy link
Contributor

@philsbln philsbln commented Sep 8, 2018

After reading #203 again, I get the feeling we actually need two properties:

  • Allow Fragmentation for the Messages passed to the transport service.
  • Allow IP level Fragmentation for the Packets generated by the transport protocol.

Now it becomes ugly:

  • For UDP, both are equivalent.
  • For TCP/QUIC, the former set to false means that sending a message should fail if the exceeds the PMTU after passing the framer. This also implies that a message should be sent within a single packet.
  • For TCP/QUIC, the latter set to false means that the transport system should set the DF flag for v4 and disable local fragmentation for v6, but the transport protocol can still split a message into multiple packets.

While the former definitely works as discussed so far, I have no idea what should happen if the latter is set to false on a message but the former is set to true…

@britram britram self-assigned this Oct 7, 2018
britram added a commit that referenced this issue Oct 7, 2018
@mwelzl
Copy link
Contributor

@mwelzl mwelzl commented Oct 8, 2018

Not sure about QUIC, but what you say here about TCP isn't right: none of this should matter to TCP, you can't control DF with it and the PMTU shouldn't be visible to the application.

@mwelzl
Copy link
Contributor

@mwelzl mwelzl commented Oct 8, 2018

More on philsbln's note above: I'm putting TCP aside, and assuming he's right about QUIC:

  1. The former set to false, but the latter to true: "a message should be sent within a single packet" - but then we want to be able to allow network-layer fragmentation? That seems strange to me.

  2. The latter set to false, but the former to true: so here we want the DF flag, but it's still ok to split messages across multiple packets. Again, that seems strange to me.

=> I struggle to see a use case for any of these combinations. My proposal would be to have only one boolean property that controls both IP- and transport-level fragmentation (allowing or forbidding both).

@britram
Copy link
Contributor Author

@britram britram commented Oct 8, 2018

while I was writing #232, I also came to think that this should be a single message context property whose implementation was context-dependent made more sense. I'm happy to rework #232 along these lines.

@gorryfair
Copy link
Contributor

@gorryfair gorryfair commented Oct 9, 2018

I may have confused people - but I too think it is OK to express this as one property. I think there are subtle differences, but such details are not important to TAPS. (Any app needing finer control will likely need to control more than this).

britram added a commit that referenced this issue Oct 10, 2018
@britram
Copy link
Contributor Author

@britram britram commented Oct 10, 2018

this alternative is in #235

britram added a commit that referenced this issue Oct 10, 2018
@britram
Copy link
Contributor Author

@britram britram commented Oct 10, 2018

closed by #235

@britram britram closed this Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants