Introduce a config option for the max UDP payload size transport parameter #4570
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4570 +/- ##
==========================================
- Coverage 85.11% 85.03% -0.08%
==========================================
Files 154 154
Lines 14843 14889 +46
==========================================
+ Hits 12633 12660 +27
- Misses 1702 1719 +17
- Partials 508 510 +2 ☔ View full report in Codecov by Sentry. |
|
Should this be an option on the |
chungthuang
force-pushed
the
chungthuang/issue-4553-max-packet-buffer-size
branch
from
a42b73e
to
3e1fff0
Compare
|
Hi @marten-seemann, I followed your suggestion to move the config option to transport and drop large packets in |
chungthuang
force-pushed
the
chungthuang/issue-4553-max-packet-buffer-size
branch
from
3e1fff0
to
1ef4d68
Compare
| @@ -398,6 +408,15 @@ func (t *Transport) handlePacket(p receivedPacket) { | |||
| t.handleNonQUICPacket(p) | |||
| return | |||
| } | |||
| if len(p.data) > int(t.MaxUDPPayloadSize) { | |||
| // Peer didn't respect our max_udp_payload_size transport parameter. Drop the packet | |||
| t.logger.Debugf("received packet size %s great than the max UDP payload %s", len(p.data), t.MaxUDPPayloadSize) | |||
There was a problem hiding this comment.
| t.logger.Debugf("received packet size %s great than the max UDP payload %s", len(p.data), t.MaxUDPPayloadSize) | |
| t.logger.Debugf("received packet size %d great than the max UDP payload %d", len(p.data), t.MaxUDPPayloadSize) |
| @@ -398,6 +408,15 @@ func (t *Transport) handlePacket(p receivedPacket) { | |||
| t.handleNonQUICPacket(p) | |||
| return | |||
| } | |||
| if len(p.data) > int(t.MaxUDPPayloadSize) { | |||
There was a problem hiding this comment.
This is inconsistent with how we handle packets larger 1452 bytes. How much effort would it be to wire this up properly, such that we pass a correctly sized buffer to the kernel? Would that create any problems with GRO?
There was a problem hiding this comment.
Are you asking if we can pipe this config directly into areas like sys_conn.go#L94, or send_stream.go#L211?
I don't think any of those areas need to change as long as we make sure that the transport parameter MaxUDPPayloadSize provided is strictly less than the MaxPacketBufferSize? I understand that the RFC expects to allow the transport parameter to be set to a maximum of 65527, but I don't think that would work anyways regarding how the internal stream frame pools are statically allocated to block sizes of 1452.
Could you also point to a specific areas of where handling packets larger than the 1452? I could be missing it completely.
Regarding the GRO comment, similarly, if we make sure that the transport parameter is strictly less than the MaxPacketBufferSize of 1452, this shouldn't be a problem?
There was a problem hiding this comment.
Are you asking if we can pipe this config directly into areas like sys_conn.go#L94, or send_stream.go#L211?
Definitely not in the send stream, but yes, in the connection. That way, the kernel would handle the size of the read buffer.
I'm wondering how different packet sizes are dealt with when using GRO: For GRO, we'll pass a 64k buffer to the kernel. Does this mean that the kernel could (in principle) return one jumbo packet, or is there any way to set a maximum packet size?
This helps address #4553 in the short term before we have #3955.