In D45253#1035534, @tuexen wrote:

In D45253#1035494, @lstewart wrote:

Might be worth adding a check in tcp_subr.c:register_tcp_functions_as_names() that the handoff_ok hook is not NULL and return an errno + fail to register the module if it is?

This is already part of the diff, I think. See tcp_subr.c:1186. tcp_subr.c:register_tcp_functions_as_names() would return EINVAL like in the case where some other mandatory function is not provided.

In D45253#1035180, @tuexen wrote:

In D45253#1035179, @lstewart wrote:

Right now, the absence of the function means that it is not OK to switch, except the state is TCPS_CLOSED. So I think keeping the semantic of the absence of the tfb_tcp_handoff_ok() is in tune with POLA for all states except TCPS_CLOSED. The other reason is that one could use tfb_tcp_handoff_ok() as the first step of the switch. In particular, one could call something like tcp_timer_stop_nounlock as the last step of tfb_tcp_handoff_ok(). This does not work, if the semantic of the absence of tfb_tcp_handoff_ok() is that it is OK to switch.

Your comments are entirely reasonable so I don't have a strong objection against what you've proposed (although the idea of mutating the connection state/set up in handoff_ok() seems sketchy at best, but we can't stop anyone from doing it either).

I think the protocol would require that if tfb_tcp_handoff_ok() returns no error, tfb_tcp_fb_init() must be called. So you can put something in tfb_tcp_handoff_ok(), which either fails and does not change the state or succeeds and potentially change the state in a way that tfb_tcp_fb_init() expects. But this is up to the user of the API and we cannot control what is done the in callbacks.

Right now, the absence of the function means that it is not OK to switch, except the state is TCPS_CLOSED. So I think keeping the semantic of the absence of the tfb_tcp_handoff_ok() is in tune with POLA for all states except TCPS_CLOSED. The other reason is that one could use tfb_tcp_handoff_ok() as the first step of the switch. In particular, one could call something like tcp_timer_stop_nounlock as the last step of tfb_tcp_handoff_ok(). This does not work, if the semantic of the absence of tfb_tcp_handoff_ok() is that it is OK to switch.

Nit: why require tfb_tcp_handoff_ok() be implemented by a stack rather than treat the absence of the hook's implementation to be an implicit ok? Many of the hook functions are optional so it seems a bit unexpected to require that this hook function be implemented.

Hi Mark,

I won't have the time to review this in the near future but would note that many of the things I flagged for the original newreno hystart++ implementation also apply to this implementation. However, I wouldn't call any of them blockers so don't have an objection to you committing this at your discretion.

The whole point of this was that there were absolutely no logs what so ever when tcp_respond() was used. So prior to this set of commits you *never* saw anything when we responded to a packet (and that happens in many places in all stacks). Now I would have to study when we use tcp_respond() with a INP_RLOCK() this is a very narrow case I would imagine, but if you are not willing to get a INP_WLOCK() that implies to me you are not changing the TCB... but thats what logging does.

Thanks for fixing!

First pass feedback

I haven't finished reviewing yet but some initial thoughts in addition to my inline comments:

• Nit: please submit diffs with full context as it makes reviewing easier
• You need to address how enabling Hystart++ when using cc_newreno with the default FreeBSD stack will work
• The implementation pessimises the common case where newreno will be used without ABE or Hystart++ which is probably undesirable. The module's lack of a cb_init() hook implementation was very deliberate to avoid such pessimisation and also as noted inline, assumptions are made elsewhere that cc_newreno can always be used as an algo of last resort so we cannot allow init to ever fail.

Loic doesn't have a commit bit, so I've offered to commit on his behalf.

In D26807#610464, @rscheff wrote:

In D26807#610449, @lstewart wrote:

In D26807#604494, @rscheff wrote:

incrementing by t_maxseg (w/o TSopt) vs. tcp_maxseg() (w/ TSopt/MPTCP) does not lead to a unexpected transmission of 2 segments, when only a single segment would be expected (tcp_output prefers to send full segments but considers option overhead).

Incrementing by tcp_maxseg() is sightly less aggressive during CA, not?

First up, it seems buggy to me that the sender side accounts for received SACK blocks in the "effective" MSS computed by tcp_maxseg() for outbound segments. (Not your bug, but your change amplifies the effect of the bug).

Not quite sure I get your point here; are you saying the tcp_maxseg() is erraneously including received SACK blocks fom the previous ACK? Maybe you can send an annotated code snippet per direct mail?

Apologies for the delayed reaction...

Sorry for the tardy comments, but a few things in this changeset caught my eye during our internal Netflix upstream merge review.

Thanks for the fix @brooks

In D20477#477820, @cem wrote:

I'm still confused why some of the structures are defined in endian-specific ways — it would be good if the author could answer that since guesses are just guesses.

Agreed and apologies, I misread/miunderstood the code. I also touched base with one of the original CDG authors to sanity check and they agree this change is a good idea.

Suggest moving stailq flush loops out of cdg_cb_destroy() into an inline function, changing smoothing_factor sysctl to a SYSCTL_PROC with custom handler similar to the exp_backoff_scale sysctl, and calling flush function from both cdg_cb_destroy() and sysctl handler when smoothing_factor is set to zero

Actually, we probably need to flush the qdiffmin_q and qdiffmax_q lists in the sample_q_size == 0 branch so that a change back to non-zero smoothing_factor doesn't start operating with stale data.

Thank you for fixing this.

Thanks all for the feedback... I'm a bit rusty on working with ports. Will commit sometime this week with my src commit bit hat on and "review/approved by:" if no further feedback materialises.

Checked with one of the authors. Preference is to reference the Bitbucket URL in pkg-descr if it can only list a single URL.

Full context diff, shuffle variable order and ditch DISTVERSION in favour of a custom variable to hold the commit hash used in the tarball directory name.

@tuexen Any thoughts on the TFO case?

In D16282#347654, @kbowling wrote:

I would prefer to leave the check in cc_cdg until we run to ground why the callback was getting hit twice (in the case someone has cc_cdg as default).

In D16282#347300, @kbowling wrote:

@lstewart we debated who should NULL before submitting this, I think it can be moved out of the destructors to the places I put the KASSERTs but I'll have to double check everything. One issue I foresaw NULLing in the framework is that we have to trust that the cc_cb_destroy destructor did indeed free() and can't catch mistakes with a KASSERT. But writing a new CC isn't exactly common and will probably be cribbed from an existing one so maybe I was over thinking that angle.

In D16282#347262, @jason_eggnet.com wrote:

If the cb_destroy virtual function shouldn't null the pointer, I assert that it shouldn't free it either.

There should be another cc_assert wrapper function that checks for not null, frees then nulls, after calling the virtual function.

When ABE was added (rS331214) to New Reno and leak fixed (rS333699) , some assumptions about the state of it as the default and always ready cc seem to have changed, notably it now has a constructor and destructor (newreno_cb_destroy) for per connection state.

Final commit candidate, rebased against FreeBSD head r333598, and with M_ZERO removed from malloc call.

newreno_plugleak_v3.diff with getsockopt(2)/setsockopt(2) updated.

@wollman Many thanks for the historical and standards related context - greatly appreciated. I realised that I probably need to update getsockopt(2)/setsockopt(2) as well...

In D15358#323834, @thj wrote:

In D15358#323759, @lstewart wrote:

Still to be tested, but I think something like this would address the leak and change memory allocation to being conditional on need: newreno_plugleak_v1.diff

This reads okay.

I ran through a loop with netcat and didn't doesn't leak(of course it wouldn't!) and tested setting the abe beta values via set sockopt with a modified netcat (https://people.freebsd.org/~thj/diffs/ncabe.diff). This also doesn't leak.

Still to be tested, but I think something like this would address the leak and change memory allocation to being conditional on need: newreno_plugleak_v1.diff

@jason_eggnet.com I thought about this some more and while there is no doubt that overflow/underflow due to the function's inputs is possible and needs to be remedied, the cause of overflow in your test case is not in fact the time since congestion being too large, but the bogus value of K, which for a wmax of 1460 bytes (i.e. slightly more than 1 MSS) should be 205 per my quick check:

Oh and @jason_eggnet.com , regarding your test code, I structured things the way they are so that you can simply #include <netinet/cc/cc_cubic.h> into your userspace test.c to get access to the various window calculation inlines rather than copy/pasting them.

This fix doesn't make sense to me. If it has been a long time since the last congestion epoch resulting in an overflow in the calculated congestion window, collapsing the window from <something_large> to 1 segment is a terrible idea. There's also no sound reason that cwnd shouldn't be allowed to shrink below 1 segment, and if that causes problems elsewhere, those places should be fixed.

yoda be gone

@cem Thanks!

Update diff post r322614 commit.

Conrad: would you be willing to sanity check this sbuf change for me as well?

In D8536#249937, @cem wrote:

Looks fine. I think it would be good to note in the commit message that this is only for top-level sections and does not nest. It's obvious when you think about it, but, I like clarity.

Rebase patch against current head and address review feedback.

The off-by-one error was incorrectly attributed to the condition that checks if vsnprintf() was successful at rendering all of the specified content into the sbuf.