I feel like this is pretty fragile. This reset reason can pass through a few layers of code, and it's not clear before this point (at least for me, a newcomer to the code) that these are the only two valid reasons (nor why they are). Also, it's not trivial to quickly/trivially walk the code paths to be certain this assert holds.

IMHO, the design would be better if it was impossible for these invalid values to be passed, either via:

1. Use a new/different enum (not an int) for only the valid reasons to reset.
2. Replace rstreason with a bool to indicate whether its bandwidth limited or not.

Then you wouldn't even need the assert. If we don't expect there to be other valid reasons in the future, I vote for the bool.

In D51721#1181893, @nickbanks_netflix.com wrote:

I feel like this is pretty fragile. This reset reason can pass through a few layers of code, and it's not clear before this point (at least for me, a newcomer to the code) that these are the only two valid reasons (nor why they are). Also, it's not trivial to quickly/trivially walk the code paths to be certain this assert holds.

IMHO, the design would be better if it was impossible for these invalid values to be passed, either via:

1. Use a new/different enum (not an int) for only the valid reasons to reset.
2. Replace rstreason with a bool to indicate whether its bandwidth limited or not.

Then you wouldn't even need the assert. If we don't expect there to be other valid reasons in the future, I vote for the bool.

Just to be clear: I am not concerned that some invalid value is passed in. The values used for rstreason are defined in icmp_var.h so I think changing them to an enum would not only affect the TCP code.
In D51440 I reduced the number of TCP counters to one. Using more than one was exposing a side channel which you could use for at least performing a stealth port scan. So the reason I added the KASSERT() is to document that you need to think if you add in the future more than one specific counter. I guess, it would be good to add a comment stating that.
I even contemplate if it is a good idea to use the BANDLIM_UNLIMITED at all. It gives a global observable signal. We would be save if we remove that...
So we could use a bool parameter. That would not allow to use a third value, but you could change it. So I guess, making a comment is what is needed. Or do the final step and remove the BANDLIM_UNLIMITED at all. I'll bring that up at the transport call this Thursday.

In D51721#1181966, @tuexen wrote:

Just to be clear: I am not concerned that some invalid value is passed in. The values used for rstreason are defined in icmp_var.h so I think changing them to an enum would not only affect the TCP code.

I was suggesting a completely new enum could be defined for this. It would only have valid values for the types you need (not everything from icmp_var.h).

In D51721#1181971, @nickbanks_netflix.com wrote:

In D51721#1181966, @tuexen wrote:

Just to be clear: I am not concerned that some invalid value is passed in. The values used for rstreason are defined in icmp_var.h so I think changing them to an enum would not only affect the TCP code.

I was suggesting a completely new enum could be defined for this. It would only have valid values for the types you need (not everything from icmp_var.h).

Hmm. Conceptually that would be a sub-enum of an enum from icmp_var.h, where the enum in icmp_var.h is not even an enum. On the other hand, the crucial point here is that you don't open a side channel. Using an enum doesn't help here, since you could just add a member to the enum. I want to make clear, that adding such a value needs some thinking. With the KASSERT() I get people to look at the comment, I hope. If someone needs additional values and double checked that there are no side channels opened, the person can change the code and the KASSERT(). At least that person must have read the comment when adding additional values. That is all that I want.