fix: respecting max_datagrams in poll_transmit

[filipe-cantarelli]

Hello,

I've been playing with quinn-proto recently and I noticed that, sometimes, when the maximum segment is large enough quinn will go over the max_datagrams limit of poll_transmit. I believe this PR will fix this issue.

Also, considering that quin-proto is a low level crate, it feels like this code:

quinn/quinn-proto/src/connection/mod.rs

Lines 454 to 457 in 434c358

	let max_datagrams = match self.config.enable_segmentation_offload {
	false => 1,
	true => max_datagrams.min(MAX_TRANSMIT_SEGMENTS),
	};

should be removed and left for the upper level code to decide what max limit they truly want to use.

[Ralith]

Also, considering that quin-proto is a low level crate, it feels like this code:

That code exists so that the application layer can easily opt out of GSO without needing to replace the UDP socket abstraction.

[filipe-cantarelli]

Also, considering that quin-proto is a low level crate, it feels like this code:

That code exists so that the application layer can easily opt out of GSO without needing to replace the UDP socket abstraction.

How about max_datagrams.min(MAX_TRANSMIT_SEGMENTS), is it possible to remove this limitation so the app code request more than 10 segments?

[Ralith]

Oh, you were concerned about the upper bound? We could probably get rid of that entirely, yeah.

[filipe-cantarelli]

Oh, you were concerned about the upper bound? We could probably get rid of that entirely, yeah.

Is this something that could be done here in this PR?

[Ralith]

Is this something that could be done here in this PR?

Sure, in a separate commit.

[djc]

(Be sure to rebase on main to appease CI.)

[filipe-cantarelli]

Is this something that could be done here in this PR?

Sure, in a separate commit.

Should we keep the current quinn behavior by moving this max cap over there? From what I see, currently linux systems would start dumping 64 segments instead of 10. Maybe set a default of max 10 in there, but also expose a way to set accordingly?

[Ralith]

Should we keep the current quinn behavior by moving this max cap over there?

That'd be safer than removing it outright, though bare quinn-proto users will still be affected. IMO we should consider relaxing it entirely, but if you're not interested in doing testing that (especially on Windows where the kernel-enforced limit is a full 512 datagrams) we could leave it to future work.

Maybe set a default of max 10 in there, but also expose a way to set accordingly?

We should avoid offering configuration knobs unless there's clear motivation to support multiple cases, i.e. evidence that no one value is adequate for the vast majority of users.

[filipe-cantarelli]

Could I then move the 10 limit to quinn in this PR and then open an issue to follow up relaxing it? I could maybe work on that later.

As far as where to cap it, maybe here is a good place:
https://github.com/quinn-rs/quinn/blob/main/quinn/src/connection.rs#L984

[Ralith]

Could I then move the 10 limit to quinn in this PR and then open an issue to follow up relaxing it? I could maybe work on that later.

SGTM, thanks!

As far as where to cap it, maybe here is a good place:

Yep, that's the spot.

[filipe-cantarelli]

@Ralith Follow up issue created #2189

[filipe-cantarelli]

@djc, @Ralith what are the next steps for this pr?

[djc]

Would be good to get another round of review from @Ralith before merging.

[flub]

Good catch. I remember looking at this and wondering that this was an odd way to check that condition, but didn't realise it would lead to this bug.

[Ralith]

Thanks!