CUBIC_SHIFT == 8: means (2 * CUBIC_SHIFT) ==16

Why double CUBIC_SHIFT left shift here?

The "target" window unit is bytes, then there shouldn't be an extra multiply of "CCV(ccv, t_maxseg)".

Rename the variable "west" to "W_est".

Also the Figure 4 in RFC 9438 equals below if we use the "bytes_acked" and "cwnd" unit in bytes. There is no need of "smss" in the calculation.

(Figure 4): W_est = W_est + Alpha_cubic * bytes_acked / cwnd

Better use UINT_MAX, instead of INT_MAX.

Better use UINT_MAX, instead of INT_MAX.

I think for some clarity of the two CC_NDUPACK and CC_ECN cases, we can rework cubic_ssthresh_update(), and rename it to cubic_get_ssthresh() like below and explicitly update CCV(ccv, snd_ssthresh).

like this:

if (!IN_CONGRECOVERY(CCV(ccv, t_flags))) {
        ssthresh = cubic_get_ssthresh(ccv, mss);
        CCV(ccv, snd_ssthresh) = max(ssthresh, 2 * maxseg);
...
}

There is a redundant pipe calculation where cubic_ssthresh_update() already did it. If use the new cubic_get_ssthresh() mentioned above, then, these three lines can be reworked to:

if (!IN_CONGRECOVERY(CCV(ccv, t_flags))) {
        ssthresh = cubic_get_ssthresh(ccv, mss);
        CCV(ccv, snd_cwnd) = ulmax(ssthresh, CCV(ccv, t_maxseg));
        CCV(ccv, snd_ssthresh) = max(ssthresh, 2 * maxseg);
...
}

May rework this cubic_ssthresh_update() function like below, for better clarity.

static uint32_t
cubic_get_ssthresh(struct cc_var *ccv, uint32_t maxseg) {
...
        return (pipe * CUBIC_BETA) >> CUBIC_SHIFT;
}

According to RFC9438 Section 4.8, CUBIC follows Reno to reduce cwnd [RFC5681] but sets ssthresh using βcubic (same as in Section 4.6), so here snd_ssthresh should be updated as:

	ssthresh = (cubic_compute_pipe(ccv) * CUBIC_BETA) >> CUBIC_SHIFT;
        CCV(ccv, snd_ssthresh) = max(ssthresh, 2 * mss);
}

According to rfc9438 section 4.10. Slow Start, K is set to 0 when exiting the first slow start without incurring any packet loss (W_max is undefined).

According to RFC9438 Section 4.8 Timeout, W_est SHOULD be set to the congestion window size at the beginning of the current congestion avoidance stage. So the code branch shall be like this:

if (cubic_data->flags & CUBICFLAG_RTO_EVENT) {
        cubic_data->flags &= ~CUBICFLAG_RTO_EVENT;
        W_est = CCV(ccv, snd_cwnd);
        cubic_data->W_max = cubic_data->W_est = W_est;
        cubic_data->K = 0;
} else {
        usecs_since_epoch = (ticks - cubic_data->t_epoch) * tick;
...
        W_est = cubic_data->W_est = tf_cwnd(...);
}

Missing both "undo_ssthresh" and "undo_cwnd" variables, and their value assignments.

But I wonder this is the place that differs from the RFC9438 Section 4.9 Spurious Congestion Events, where these "undo_*" variables are used for Spurious Fast Retransmits (section 4.9.2.) and only if "cwnd < cwnd_prior".

CUBIC_SHIFT == 8: means (2 * CUBIC_SHIFT) ==16

Why double CUBIC_SHIFT left shift here?

On a second understanding, the old ONE_SUB_CUBIC_BETA (which contains 8bits left shift) is removed in the new formula. Therefore, adding the 8bits left shift makes sense now.

Rename the variable "west" to "W_est".

Assuming the "cwnd" has a unit of byte, then the "smss" variable is no longer needed as in Figure 4.
(Figure 4): W_est = W_est + Alpha_cubic * bytes_acked / cwnd

That section in 4.10 talks about the interaction with hystart++, when existing without loss; the base stack doesn't do hystart++, and exists slow start only after loss or ECN mark - with a defined cwnd at that moment of a congestion signal.

the tp->ssthresh_prev are unrolled in the generic CC_RTO_ERR handling, before the CC specific callout is done...
Also, while FBSD emits DSACK when duplicate data segments are received, that information is currently not processed on receipt - so there is no spurious fast retransmit detection yet. (section 4.9.2 doesn't apply).

I think this input variable unsigned long smss is no longer needed.

Can we make this file's change into a separate patch? It does not look to be related with the CUBIC algorithm. Am I right?