tcp: Make RFC 6191 support configurable
ClosedPublic
Actions

Authored by des on Sun, May 17, 1:02 PM.

Details

Reviewers

marius.h_lden.org
pouria
tuexen

Group Reviewers

Klara
transport

Commits

rG2af70d7a3849: tcp: Make RFC 6191 support configurable

Summary

Add a default-on per-VIMAGE sysctl for RFC 6191 connection recycling.
This makes it possible to merge the change to older branches where it
can be switched off by default to minimize risk.

MFC after: 1 week
Sponsored by: Klara, Inc.
Sponsored by: Modirum MDPay

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

des created this revision.Sun, May 17, 1:02 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptSun, May 17, 1:02 PM

Herald added subscribers: ziaee, glebius, melifaro, imp. · View Herald Transcript

des requested review of this revision.Sun, May 17, 1:02 PM

Harbormaster completed remote builds in B73160: Diff 177963.Sun, May 17, 1:02 PM

swap && to put cheaper term first

Harbormaster completed remote builds in B73161: Diff 177964.Sun, May 17, 1:04 PM

LGTM

tuexen accepted this revision.Sun, May 17, 4:12 PM

This revision is now accepted and ready to land.Sun, May 17, 4:12 PM

Please note that the support of RFC 6191 has been MFC'ed to stable/14 and stable/15. I did not use a sysctl variable, since I don't see a need for adding the configuration complexity for this feature. Why would you want to not use it?

In D57045#1307167, @tuexen wrote:

Please note that the support of RFC 6191 has been MFC'ed to stable/14 and stable/15. I did not use a sysctl variable, since I don't see a need for adding the configuration complexity for this feature. Why would you want to not use it?

Our intention is to merge this to 15 and 14 as well, then do an EN with the combined change.

In D57045#1307179, @des wrote:

In D57045#1307167, @tuexen wrote:

Please note that the support of RFC 6191 has been MFC'ed to stable/14 and stable/15. I did not use a sysctl variable, since I don't see a need for adding the configuration complexity for this feature. Why would you want to not use it?

Our intention is to merge this to 15 and 14 as well, then do an EN with the combined change.

What is the errata about? Did my change introduce a bug? Adding the sysctl-variable doubles the testing effort. Not sure why we need it. Could you elaborate?

I don't know how to withdraw my approval, but, right now, I am not supportive of this change. (I am also not objecting).

In D57045#1307188, @tuexen wrote:

In D57045#1307179, @des wrote:

In D57045#1307167, @tuexen wrote:

Please note that the support of RFC 6191 has been MFC'ed to stable/14 and stable/15. I did not use a sysctl variable, since I don't see a need for adding the configuration complexity for this feature. Why would you want to not use it?

Our intention is to merge this to 15 and 14 as well, then do an EN with the combined change.

What is the errata about? Did my change introduce a bug? Adding the sysctl-variable doubles the testing effort. Not sure why we need it. Could you elaborate?

Ideally we deprecate the sysctl in the medium-term, but the desire is that we have the RFC6191 behavior in release branches to be able to solve the bug originally reported. There's some desire to be more conservative since there have been few known reports of bugs with the previous behavior, so the knob allows it to be retrofitted without breaking anyone.

I don't know how to withdraw my approval, but, right now, I am not supportive of this change. (I am also not objecting).

In D57045#1307541, @kevans wrote:

In D57045#1307188, @tuexen wrote:

In D57045#1307179, @des wrote:

In D57045#1307167, @tuexen wrote:

Please note that the support of RFC 6191 has been MFC'ed to stable/14 and stable/15. I did not use a sysctl variable, since I don't see a need for adding the configuration complexity for this feature. Why would you want to not use it?

Our intention is to merge this to 15 and 14 as well, then do an EN with the combined change.

What is the errata about? Did my change introduce a bug? Adding the sysctl-variable doubles the testing effort. Not sure why we need it. Could you elaborate?

Ideally we deprecate the sysctl in the medium-term, but the desire is that we have the RFC6191 behavior in release branches to be able to solve the bug originally reported. There's some desire to be more conservative since there have been few known reports of bugs with the previous behavior, so the knob allows it to be retrofitted without breaking anyone.

Are there any reports that the new behavior is problematic? The current behavior is not a bug. Adding support RFC 6191 is adding a feature which will be in upcoming 15 and 14 releases. Are we using ENs to add features?
Which existing versions need this additional features? Can't users upgrade to new versions? It won't be too long until 15.1 will be released.

I don't know how to withdraw my approval, but, right now, I am not supportive of this change. (I am also not objecting).

In D57045#1307542, @tuexen wrote:

In D57045#1307541, @kevans wrote:

In D57045#1307188, @tuexen wrote:

In D57045#1307179, @des wrote:

In D57045#1307167, @tuexen wrote:

Please note that the support of RFC 6191 has been MFC'ed to stable/14 and stable/15. I did not use a sysctl variable, since I don't see a need for adding the configuration complexity for this feature. Why would you want to not use it?

Our intention is to merge this to 15 and 14 as well, then do an EN with the combined change.

What is the errata about? Did my change introduce a bug? Adding the sysctl-variable doubles the testing effort. Not sure why we need it. Could you elaborate?

Ideally we deprecate the sysctl in the medium-term, but the desire is that we have the RFC6191 behavior in release branches to be able to solve the bug originally reported. There's some desire to be more conservative since there have been few known reports of bugs with the previous behavior, so the knob allows it to be retrofitted without breaking anyone.

Are there any reports that the new behavior is problematic? The current behavior is not a bug. Adding support RFC 6191 is adding a feature which will be in upcoming 15 and 14 releases. Are we using ENs to add features?

The pre-6191 behavior is a bug for the folks that originally requested it, because it results in significant drops that should be avoided. The new feature that avoids those drops is incredibly light and easy to keep disabled for the release branches only, thus the knob that will get its default flipped for existing releases (but maintain current state in stable and main).

Which existing versions need this additional features? Can't users upgrade to new versions? It won't be too long until 15.1 will be released.

This is being incredibly optimistic about moving corporate infrastructure to new releases- in this case, the move is in progress but change control means their timelines have limitations that mean they won't be able to hit 15.1 for months, so being able to avoid the original bug without impacting anyone else would be much appreciated.

In D57045#1307543, @kevans wrote:

In D57045#1307542, @tuexen wrote:

In D57045#1307541, @kevans wrote:

In D57045#1307188, @tuexen wrote:

In D57045#1307179, @des wrote:

In D57045#1307167, @tuexen wrote:

Please note that the support of RFC 6191 has been MFC'ed to stable/14 and stable/15. I did not use a sysctl variable, since I don't see a need for adding the configuration complexity for this feature. Why would you want to not use it?

Our intention is to merge this to 15 and 14 as well, then do an EN with the combined change.

What is the errata about? Did my change introduce a bug? Adding the sysctl-variable doubles the testing effort. Not sure why we need it. Could you elaborate?

Ideally we deprecate the sysctl in the medium-term, but the desire is that we have the RFC6191 behavior in release branches to be able to solve the bug originally reported. There's some desire to be more conservative since there have been few known reports of bugs with the previous behavior, so the knob allows it to be retrofitted without breaking anyone.

Are there any reports that the new behavior is problematic? The current behavior is not a bug. Adding support RFC 6191 is adding a feature which will be in upcoming 15 and 14 releases. Are we using ENs to add features?

The pre-6191 behavior is a bug for the folks that originally requested it, because it results in significant drops that should be avoided. The new feature that avoids those drops is incredibly light and easy to keep disabled for the release branches only, thus the knob that will get its default flipped for existing releases (but maintain current state in stable and main).

I don't think it is a bug, since it is completely covered by the TCP RFCs. Adding support for RFC 6191 is adding a new feature. And it is already in all supported stable branches. So why do we need a way to go back to the old behavior?

Just to be clear: I am just asking why we need to add a knob to get back the old behavior? I see no reason why you would switch to the old behavior. That is why I MFCed it without adding a knob.

Which existing versions need this additional features? Can't users upgrade to new versions? It won't be too long until 15.1 will be released.

This is being incredibly optimistic about moving corporate infrastructure to new releases- in this case, the move is in progress but change control means their timelines have limitations that mean they won't be able to hit 15.1 for months, so being able to avoid the original bug without impacting anyone else would be much appreciated.

OK. So you want to have an EN to add a feature to 15.0? Why do you need to complexity of the sysctl? Why can't you just take what is in the main, stable/15, and stable/14 branch?

Just to be clear: I like the support of RFC 6191. I just want to understand why you don't want to have it and therefore need a sysctl variable?

Can't the people provide a patch for their system or do they need an EN? If they need an EN, which do you need something you can disable? Wouldn't it be acceptable to just enable support for RFC 6191 unconditionally?

In D57045#1307566, @tuexen wrote:

OK. So you want to have an EN to add a feature to 15.0? Why do you need to complexity of the sysctl? Why can't you just take what is in the main, stable/15, and stable/14 branch?

Just to be clear: I like the support of RFC 6191. I just want to understand why you don't want to have it and therefore need a sysctl variable?

Because ENs should be conservative: users applying changes with freebsd-update cannot pick and choose the patches they apply. So changes shipped that way should aim to do as little as possible.

Can't the people provide a patch for their system or do they need an EN? If they need an EN, which do you need something you can disable? Wouldn't it be acceptable to just enable support for RFC 6191 unconditionally?

I am not convinced that we need or want this sysctl in main, stable/* etc.. But if we're going to ship the change in an EN, I think we should avoid changing the behaviour by default in releng/*; those that explicitly want RFC 6191 support can opt in. That's why I (on behalf of secteam) asked for the sysctl.

In D57045#1307570, @markj wrote:

In D57045#1307566, @tuexen wrote:

OK. So you want to have an EN to add a feature to 15.0? Why do you need to complexity of the sysctl? Why can't you just take what is in the main, stable/15, and stable/14 branch?

Just to be clear: I like the support of RFC 6191. I just want to understand why you don't want to have it and therefore need a sysctl variable?

Because ENs should be conservative: users applying changes with freebsd-update cannot pick and choose the patches they apply. So changes shipped that way should aim to do as little as possible.

Can't the people provide a patch for their system or do they need an EN? If they need an EN, which do you need something you can disable? Wouldn't it be acceptable to just enable support for RFC 6191 unconditionally?

I am not convinced that we need or want this sysctl in main, stable/* etc.. But if we're going to ship the change in an EN, I think we should avoid changing the behaviour by default in releng/*; those that explicitly want RFC 6191 support can opt in. That's why I (on behalf of secteam) asked for the sysctl.

OK. So we want to deliver a new feature, off by default, in a EN to released versions and allow people to enable it. I just wasn't aware that we do this. But shouldn't we then also disable this feature in stable branches? In particular, should we disable this for 15.1? I can revert my unconditional enabling of RFC 6191 support and ask re@ to merge that into releng/15.1.

I would definitely argue that not implementing a 15-year-old IETF BCP is a bug.

In D57045#1307588, @tuexen wrote:

OK. So we want to deliver a new feature, off by default, in a EN to released versions and allow people to enable it. I just wasn't aware that we do this. But shouldn't we then also disable this feature in stable branches? In particular, should we disable this for 15.1? I can revert my unconditional enabling of RFC 6191 support and ask re@ to merge that into releng/15.1.

No, we leave it on by default in 15.1, but flip the default in the EN patch so 15.0 and 14.[45] get it off by default. This way users who aren't currently experiencing any issues are unaffected, and those that are can turn it on.

In D57045#1307588, @tuexen wrote:

In D57045#1307570, @markj wrote:

In D57045#1307566, @tuexen wrote:

OK. So you want to have an EN to add a feature to 15.0? Why do you need to complexity of the sysctl? Why can't you just take what is in the main, stable/15, and stable/14 branch?

Just to be clear: I like the support of RFC 6191. I just want to understand why you don't want to have it and therefore need a sysctl variable?

Because ENs should be conservative: users applying changes with freebsd-update cannot pick and choose the patches they apply. So changes shipped that way should aim to do as little as possible.

Can't the people provide a patch for their system or do they need an EN? If they need an EN, which do you need something you can disable? Wouldn't it be acceptable to just enable support for RFC 6191 unconditionally?

I am not convinced that we need or want this sysctl in main, stable/* etc.. But if we're going to ship the change in an EN, I think we should avoid changing the behaviour by default in releng/*; those that explicitly want RFC 6191 support can opt in. That's why I (on behalf of secteam) asked for the sysctl.

OK. So we want to deliver a new feature, off by default, in a EN to released versions and allow people to enable it. I just wasn't aware that we do this. But shouldn't we then also disable this feature in stable branches? In particular, should we disable this for 15.1? I can revert my unconditional enabling of RFC 6191 support and ask re@ to merge that into releng/15.1.

I don't really see why, unless the change is particularly risky. Users upgrading to 15.1 or tracking stable have more options if the upgrade causes problems, they can roll back to 15.0 or revert a particular patch etc.. Users applying SA/EN patches have fewer options, so we should avoid publishing ENs unless we're very confident in the change. In this case, the change seems delicate enough that we shouldn't automatically enable it.

In D57045#1307590, @des wrote:

I would definitely argue that not implementing a 15-year-old IETF BCP is a bug.

OK. So I learn that implementing a BCP is considered a bugfix and can be brought in via an EN.

In D57045#1307593, @markj wrote:

In D57045#1307588, @tuexen wrote:

In D57045#1307570, @markj wrote:

In D57045#1307566, @tuexen wrote:

OK. So you want to have an EN to add a feature to 15.0? Why do you need to complexity of the sysctl? Why can't you just take what is in the main, stable/15, and stable/14 branch?

Just to be clear: I like the support of RFC 6191. I just want to understand why you don't want to have it and therefore need a sysctl variable?

Because ENs should be conservative: users applying changes with freebsd-update cannot pick and choose the patches they apply. So changes shipped that way should aim to do as little as possible.

Can't the people provide a patch for their system or do they need an EN? If they need an EN, which do you need something you can disable? Wouldn't it be acceptable to just enable support for RFC 6191 unconditionally?

I am not convinced that we need or want this sysctl in main, stable/* etc.. But if we're going to ship the change in an EN, I think we should avoid changing the behaviour by default in releng/*; those that explicitly want RFC 6191 support can opt in. That's why I (on behalf of secteam) asked for the sysctl.

OK. So we want to deliver a new feature, off by default, in a EN to released versions and allow people to enable it. I just wasn't aware that we do this. But shouldn't we then also disable this feature in stable branches? In particular, should we disable this for 15.1? I can revert my unconditional enabling of RFC 6191 support and ask re@ to merge that into releng/15.1.

I don't really see why, unless the change is particularly risky. Users upgrading to 15.1 or tracking stable have more options if the upgrade causes problems, they can roll back to 15.0 or revert a particular patch etc.. Users applying SA/EN patches have fewer options, so we should avoid publishing ENs unless we're very confident in the change. In this case, the change seems delicate enough that we shouldn't automatically enable it.

OK, up to re@ and secteam@. But I find it strange that we add a new features in a switchable way to 15.0 and have it not switchable in 15.1. If you think it is delicate enough, shouldn't we

have it switchable in main, default off.
MFC it to stable/14 and stable/15.
MFC it to releng/15.1.
Add ENs to the supported versions.
After that, switch it on for 16 and mark that the sysctl is deprecated.
remove the sysctl in 17.

marius.h_lden.org accepted this revision.Mon, May 18, 2:32 PM

In D57045#1307615, @tuexen wrote:

In D57045#1307590, @des wrote:

I would definitely argue that not implementing a 15-year-old IETF BCP is a bug.

OK. So I learn that implementing a BCP is considered a bugfix and can be brought in via an EN.

There are no hard rules for determining whether a bugfix should be published as an EN. It's a judgement call:

Does the change fix a widely reported problem? If only a few users seem to need the change (e.g., the patched feature is not widely used), we are less likely to issue an EN.
Does the problem have a workaround? If so, then the pressure to publish an EN is lower.
Is the patch complex and likely to introduce regressions or change behaviour in a surprising way? If so, we will be less comfortable publishing it.

In D57045#1307623, @markj wrote:

In D57045#1307615, @tuexen wrote:

In D57045#1307590, @des wrote:

I would definitely argue that not implementing a 15-year-old IETF BCP is a bug.

OK. So I learn that implementing a BCP is considered a bugfix and can be brought in via an EN.

There are no hard rules for determining whether a bugfix should be published as an EN. It's a judgement call:

Does the change fix a widely reported problem? If only a few users seem to need the change (e.g., the patched feature is not widely used), we are less likely to issue an EN.

Does the problem have a workaround? If so, then the pressure to publish an EN is lower.

Is the patch complex and likely to introduce regressions or change behaviour in a surprising way? If so, we will be less comfortable publishing it.