Page MenuHomeFreeBSD
Differential D29404

xen: introduce XEN_CPUID_TO_VCPUID()
ClosedPublic
Actions

Authored by ehem_freebsd_m5p.com on Mar 23 2021, 10:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 21, 12:58 AM
Unknown Object (File)
Fri, Mar 7, 11:23 AM
Unknown Object (File)
Feb 27 2025, 12:55 PM
Unknown Object (File)
Feb 23 2025, 9:11 PM
Unknown Object (File)
Feb 10 2025, 4:01 AM
Unknown Object (File)
Jan 30 2025, 4:51 AM
Unknown Object (File)
Jan 24 2025, 5:03 PM
Unknown Object (File)
Jan 24 2025, 9:27 AM
View All 100 Files
Subscribers
imp
julien_xen.org

Details

Reviewers
royger
mhorne
Commits
rG28a78d860e46: xen: introduce XEN_CPUID_TO_VCPUID()/XEN_VCPUID()
Summary

Part of the series for allowing FreeBSD/ARM to run on Xen. On ARM the
function is a trivial pass-through, other architectures need distinct
implementations.

Submitted by: Elliott Mitchell <ehem+freebsd@m5p.com>
Original implementation: Julien Grall <julien@xen.org>, 2014-04-19 08:57:40
Original implementation: Julien Grall <julien@xen.org>, 2014-04-19 14:32:01

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45560
Build 42448: arc lint + arc unit

Event Timeline

ehem_freebsd_m5p.com created this revision.Mar 23 2021, 10:04 PM
Herald added a subscriber: imp. · View Herald TranscriptMar 23 2021, 10:04 PM
ehem_freebsd_m5p.com requested review of this revision.Mar 23 2021, 10:04 PM
Harbormaster completed remote builds in B38033: Diff 86227.Mar 23 2021, 10:05 PM
royger added a comment.Mar 25 2021, 3:25 PM

I'm not sure why this is needed, as I would expect that just using PCPU_GET(vcpu_id) or PCPU_ID_GET(cpu, vcpu_id) if you need to fetch the vcpu id of a different CPU to be enough.

Maybe there's something that I'm missing, but I would say the cpu_to_vcpu_id helper is not needed at all.

sys/dev/xen/debug/debug.c
82

I think I'm confused, why do you need to fetch the cpuid to get the vcpu id? The previous code already gives you the vcpu id directly using PCPU_GET(vcpu_id).

ehem_freebsd_m5p.com added a subscriber: julien_xen.org.Mar 30 2021, 6:47 PM
ehem_freebsd_m5p.com mentioned this in D29498: xen/arm64: introduce sys/arm64/include/xen/xen-os.h.Mar 30 2021, 7:09 PM
ehem_freebsd_m5p.com added a comment.Apr 21 2021, 2:12 AM

I hope Julien Grall can remember why this was originally done. I can't really speak to this part.

mhorne requested changes to this revision.Apr 24 2021, 8:15 PM

Agreed with @royger, this appears to be an indirect way of achieving the same behaviour.

This revision now requires changes to proceed.Apr 24 2021, 8:15 PM
julien_xen.org added inline comments.Apr 25 2021, 12:37 PM
sys/dev/xen/debug/debug.c
82

This is where the implementation for the Arm counterpart could help to understand the approach :).

I don't know why on x86 you need a specific field vcpu_id. On Arm the vCPU ID will be equivalent to the physical CPU ID.

The field pc_vcpu_id doesn't seem to exist on Arm today and it looks like on x86 it is only used by Xen code. So to me it sounds a bit wasteful to introduce the field on Arm.

royger added inline comments.Apr 27 2021, 10:05 AM
sys/dev/xen/debug/debug.c
82

This is where the implementation for the Arm counterpart could help to understand the approach :).

I don't know why on x86 you need a specific field vcpu_id. On Arm the vCPU ID will be equivalent to the physical CPU ID.

On x86 the vCPU ID should be fetched from the Xen cpuid leaves. While currently there's a relation between the ACPI CPU ID and the vCPU ID, it's not written down anywhere, and a proper implementation should fetch the vCPU ID from cpuid and expect it to be a random value.

The field pc_vcpu_id doesn't seem to exist on Arm today and it looks like on x86 it is only used by Xen code. So to me it sounds a bit wasteful to introduce the field on Arm.

Fine. TBH I don't think adding an unsigned int to each CPU data area is going to make that much of a difference (it would be conditional to enabling XENHVM).

I don't seem to be able to find how the vCPU ID is fetched on Arm from the public headers, but as long as it's clear that's fine. I'm however slightly worried that we will now have a mix of cpu_to_vcpu_id and PCPU_GET(vcpu_id), which makes the code confusing IMO, that's another reason why I would prefer Arm to just add a vcpu_id field.

sys/x86/include/xen/xen-os.h
55

PCPU_ID_GET please.

royger added a comment.Apr 27 2021, 10:07 AM

If this patch has to make progress the commit message needs to be updated to reflect why a vCPU ID filed might not be needed, and that having a per-arch helper allows to abstract away whether it's a completely different CPU field or just abstracted away from existing data.

ehem_freebsd_m5p.com updated this revision to Diff 91038.Jun 17 2021, 10:06 PM

Updating, definitely taking care of one comment, hopefully working towards others. Squashing the implementation commit since the rest of that disappeared with PVHv1.

Harbormaster completed remote builds in B39964: Diff 91038.Jun 17 2021, 10:06 PM
ehem_freebsd_m5p.com edited the summary of this revision. (Show Details)Jun 17 2021, 10:07 PM
ehem_freebsd_m5p.com retitled this revision from xen: introduce cpu_to_vcpu_id() to xen: introduce PCPU_GET().
ehem_freebsd_m5p.com marked an inline comment as done.Jun 17 2021, 10:25 PM

Renamed to PCPU_GET() as requested (wondering if VCPU_GET() might be better).

I've squashed the commit where this was put into place, hopefully the greater context makes it overt the direction things go. Plus mentioning for aarch64 the function is a straight pass-through.

ehem_freebsd_m5p.com added a child revision: D30950: xen/arm64: enable arm64 Xen configuration.Jun 30 2021, 1:28 AM
ehem_freebsd_m5p.com retitled this revision from xen: introduce PCPU_GET() to xen: introduce PCPU_ID_GET().Jun 30 2021, 3:31 AM
ehem_freebsd_m5p.com edited the summary of this revision. (Show Details)
ehem_freebsd_m5p.com added a child revision: D29498: xen/arm64: introduce sys/arm64/include/xen/xen-os.h.Jul 3 2021, 12:40 AM
ehem_freebsd_m5p.com removed a child revision: D30950: xen/arm64: enable arm64 Xen configuration.Jul 3 2021, 12:43 AM
ehem_freebsd_m5p.com updated this revision to Diff 105934.May 13 2022, 2:55 AM

Updating Phabricator's in light of this having okay via e-mail. The function got renamed, but it is still pretty similar to what was present before.

Harbormaster completed remote builds in B45560: Diff 105934.May 13 2022, 2:55 AM
ehem_freebsd_m5p.com retitled this revision from xen: introduce PCPU_ID_GET() to xen: introduce XEN_CPUID_TO_VCPUID().May 13 2022, 2:57 AM
ehem_freebsd_m5p.com edited the summary of this revision. (Show Details)

This has an e-mail okay by @royger, just noticed the reject from before is still here.

mhorne added inline comments.Jun 10 2022, 8:36 PM
sys/xen/xen-os.h
47

This should just be PCPU_GET(vcpu_id). Otherwise you are fetching from the PCPU region twice when you don't need to.

If for ARM cpuid == vcpuid, then you can define XEN_VCPUID separately in each arch's xen-os.h header.

royger requested changes to this revision.Mar 24 2023, 3:05 PM

You will likely need to introduce a wrapper for xen_intr_assign_cpu() in order to keep the second parameter as an apic_id.

sys/dev/xen/bus/xen_intr.c
881

I'm afraid you cannot just change the parameter here from an apic_id into a cpuid, as this function is used by xen_intr_pic in order to fill the pic_assign_cpu hook, and that expects the second parameter of the hook to be an apic_id.

This revision now requires changes to proceed.Mar 24 2023, 3:05 PM
ehem_freebsd_m5p.com updated this revision to Diff 119402.Mar 24 2023, 4:23 PM

Restoring a lost hunk. Seems as some point I lost the xen_intr_pic_assign_cpu() function. Helpfully it wasn't lost from a later point.

Harbormaster completed remote builds in B50569: Diff 119402.Mar 24 2023, 4:23 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 14 2023, 2:01 PM
Closed by commit rG28a78d860e46: xen: introduce XEN_CPUID_TO_VCPUID()/XEN_VCPUID() (authored by julien_xen.org, committed by royger). · Explain Why
This revision was automatically updated to reflect the committed changes.
royger added a commit: rG28a78d860e46: xen: introduce XEN_CPUID_TO_VCPUID()/XEN_VCPUID().

Revision Contents
Changeset List

PathSize
sys/
dev/
xen/
bus/
26 lines
debug/
2 lines
x86/
include/
xen/
9 lines
xen/
2 lines

Diff 105934

View Options

sys/dev/xen/bus/xen_intr.c

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines#endif
* fall back to non RFC1323 RTT calculation. Normalize * fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection * timestamp if syncookies were used when this connection
* was established. * was established.
*/ */
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) { if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset; to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) { if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0; to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
} }
} }
/* /*
* Process options only when we get SYN/ACK back. The SYN case * Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache. * for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN> * According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled. * or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up. * XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 && (to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) { TAILQ_EMPTY(&tp->snd_holes)) {
/* /*
* This is a pure ack for outstanding data. * This is a pure ack for outstanding data.
*/ */
TCPSTAT_INC(tcps_predack); TCPSTAT_INC(tcps_predack);
/* /*
* "bad retransmit" recovery without timestamps. * "bad retransmit" recovery.
*/ */
if ((to.to_flags & TOF_TS) == 0 && if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID && tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 && tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) { (((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
((to.to_flags & TOF_TS) == 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR); cc_cong_signal(tp, th, CC_RTO_ERR);
}
/* /*
* Recalculate the transmit timer / rtt. * Recalculate the transmit timer / rtt.
* *
* Some boxes send broken timestamp replies * Some boxes send broken timestamp replies
rscheffUnsubmitted
Done
Inline Actions

Any specific reason to write the or statement as two separate if clauses?

I believe in other parts we do it ((true & x & y & z) | (false & a & b &c )) - as multiline checks;

I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good.

The logic looks ok to me.

rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
glebiusUnsubmitted
Done
Inline Actions

I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading.

Oh, the upper level embracing if can also be merged into one big clause:

if ((tp->t_rxtshift == 1 &&
    tp->t_flags & TF_PREVVALID &&
    tp->t_badrxtwin != 0) &&
    (((to.to_flags & TOF_TS) != 0 &&
    (to.to_tsecr != 0) &&
    TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
    ((to.to_flags & TOF_TS) == 0 &&
    TSTMP_LT(ticks, tp->t_badrxtwin))))
        cc_cong_signal(tp, th, CC_RTO_ERR);

This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b || c)).

glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
tuexenAuthorUnsubmitted
Done
Inline Actions

Now using one and two spaces of indentation to make the expression more readable.

tuexen: Now using one and two spaces of indentation to make the expression more readable.
tuexenAuthorUnsubmitted
Done
Inline Actions

It was a try to break things up for improved readability by humans.

tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore * during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a * timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer. * huge RTT and blow up the retransmit timer.
*/ */
if ((to.to_flags & TOF_TS) != 0 && if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) { to.to_tsecr) {
uint32_t t; uint32_t t;
▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines
View Options

sys/dev/xen/debug/debug.c

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines#endif
* fall back to non RFC1323 RTT calculation. Normalize * fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection * timestamp if syncookies were used when this connection
* was established. * was established.
*/ */
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) { if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset; to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) { if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0; to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
} }
} }
/* /*
* Process options only when we get SYN/ACK back. The SYN case * Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache. * for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN> * According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled. * or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up. * XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 && (to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) { TAILQ_EMPTY(&tp->snd_holes)) {
/* /*
* This is a pure ack for outstanding data. * This is a pure ack for outstanding data.
*/ */
TCPSTAT_INC(tcps_predack); TCPSTAT_INC(tcps_predack);
/* /*
* "bad retransmit" recovery without timestamps. * "bad retransmit" recovery.
*/ */
if ((to.to_flags & TOF_TS) == 0 && if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID && tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 && tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) { (((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
((to.to_flags & TOF_TS) == 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR); cc_cong_signal(tp, th, CC_RTO_ERR);
}
/* /*
* Recalculate the transmit timer / rtt. * Recalculate the transmit timer / rtt.
* *
* Some boxes send broken timestamp replies * Some boxes send broken timestamp replies
rscheffUnsubmitted
Done
Inline Actions

Any specific reason to write the or statement as two separate if clauses?

I believe in other parts we do it ((true & x & y & z) | (false & a & b &c )) - as multiline checks;

I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good.

The logic looks ok to me.

rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
glebiusUnsubmitted
Done
Inline Actions

I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading.

Oh, the upper level embracing if can also be merged into one big clause:

if ((tp->t_rxtshift == 1 &&
    tp->t_flags & TF_PREVVALID &&
    tp->t_badrxtwin != 0) &&
    (((to.to_flags & TOF_TS) != 0 &&
    (to.to_tsecr != 0) &&
    TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
    ((to.to_flags & TOF_TS) == 0 &&
    TSTMP_LT(ticks, tp->t_badrxtwin))))
        cc_cong_signal(tp, th, CC_RTO_ERR);

This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b || c)).

glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
tuexenAuthorUnsubmitted
Done
Inline Actions

Now using one and two spaces of indentation to make the expression more readable.

tuexen: Now using one and two spaces of indentation to make the expression more readable.
tuexenAuthorUnsubmitted
Done
Inline Actions

It was a try to break things up for improved readability by humans.

tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore * during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a * timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer. * huge RTT and blow up the retransmit timer.
*/ */
if ((to.to_flags & TOF_TS) != 0 && if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) { to.to_tsecr) {
uint32_t t; uint32_t t;
▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines
View Options

sys/x86/include/xen/xen-os.h

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines#endif
* fall back to non RFC1323 RTT calculation. Normalize * fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection * timestamp if syncookies were used when this connection
* was established. * was established.
*/ */
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) { if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset; to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) { if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0; to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
} }
} }
/* /*
* Process options only when we get SYN/ACK back. The SYN case * Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache. * for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN> * According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled. * or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up. * XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 && (to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) { TAILQ_EMPTY(&tp->snd_holes)) {
/* /*
* This is a pure ack for outstanding data. * This is a pure ack for outstanding data.
*/ */
TCPSTAT_INC(tcps_predack); TCPSTAT_INC(tcps_predack);
/* /*
* "bad retransmit" recovery without timestamps. * "bad retransmit" recovery.
*/ */
if ((to.to_flags & TOF_TS) == 0 && if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID && tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 && tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) { (((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
((to.to_flags & TOF_TS) == 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR); cc_cong_signal(tp, th, CC_RTO_ERR);
}
/* /*
* Recalculate the transmit timer / rtt. * Recalculate the transmit timer / rtt.
* *
* Some boxes send broken timestamp replies * Some boxes send broken timestamp replies
rscheffUnsubmitted
Done
Inline Actions

Any specific reason to write the or statement as two separate if clauses?

I believe in other parts we do it ((true & x & y & z) | (false & a & b &c )) - as multiline checks;

I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good.

The logic looks ok to me.

rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
glebiusUnsubmitted
Done
Inline Actions

I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading.

Oh, the upper level embracing if can also be merged into one big clause:

if ((tp->t_rxtshift == 1 &&
    tp->t_flags & TF_PREVVALID &&
    tp->t_badrxtwin != 0) &&
    (((to.to_flags & TOF_TS) != 0 &&
    (to.to_tsecr != 0) &&
    TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
    ((to.to_flags & TOF_TS) == 0 &&
    TSTMP_LT(ticks, tp->t_badrxtwin))))
        cc_cong_signal(tp, th, CC_RTO_ERR);

This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b || c)).

glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
tuexenAuthorUnsubmitted
Done
Inline Actions

Now using one and two spaces of indentation to make the expression more readable.

tuexen: Now using one and two spaces of indentation to make the expression more readable.
tuexenAuthorUnsubmitted
Done
Inline Actions

It was a try to break things up for improved readability by humans.

tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore * during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a * timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer. * huge RTT and blow up the retransmit timer.
*/ */
if ((to.to_flags & TOF_TS) != 0 && if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) { to.to_tsecr) {
uint32_t t; uint32_t t;
▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines
View Options

sys/xen/xen-os.h

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines#endif
* fall back to non RFC1323 RTT calculation. Normalize * fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection * timestamp if syncookies were used when this connection
* was established. * was established.
*/ */
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) { if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset; to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) { if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0; to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
} }
} }
/* /*
* Process options only when we get SYN/ACK back. The SYN case * Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache. * for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN> * According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled. * or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up. * XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 && (to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) { TAILQ_EMPTY(&tp->snd_holes)) {
/* /*
* This is a pure ack for outstanding data. * This is a pure ack for outstanding data.
*/ */
TCPSTAT_INC(tcps_predack); TCPSTAT_INC(tcps_predack);
/* /*
* "bad retransmit" recovery without timestamps. * "bad retransmit" recovery.
*/ */
if ((to.to_flags & TOF_TS) == 0 && if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID && tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 && tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) { (((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
((to.to_flags & TOF_TS) == 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR); cc_cong_signal(tp, th, CC_RTO_ERR);
}
/* /*
* Recalculate the transmit timer / rtt. * Recalculate the transmit timer / rtt.
* *
* Some boxes send broken timestamp replies * Some boxes send broken timestamp replies
rscheffUnsubmitted
Done
Inline Actions

Any specific reason to write the or statement as two separate if clauses?

I believe in other parts we do it ((true & x & y & z) | (false & a & b &c )) - as multiline checks;

I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good.

The logic looks ok to me.

rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
glebiusUnsubmitted
Done
Inline Actions

I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading.

Oh, the upper level embracing if can also be merged into one big clause:

if ((tp->t_rxtshift == 1 &&
    tp->t_flags & TF_PREVVALID &&
    tp->t_badrxtwin != 0) &&
    (((to.to_flags & TOF_TS) != 0 &&
    (to.to_tsecr != 0) &&
    TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
    ((to.to_flags & TOF_TS) == 0 &&
    TSTMP_LT(ticks, tp->t_badrxtwin))))
        cc_cong_signal(tp, th, CC_RTO_ERR);

This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b || c)).

glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
tuexenAuthorUnsubmitted
Done
Inline Actions

Now using one and two spaces of indentation to make the expression more readable.

tuexen: Now using one and two spaces of indentation to make the expression more readable.
tuexenAuthorUnsubmitted
Done
Inline Actions

It was a try to break things up for improved readability by humans.

tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore * during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a * timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer. * huge RTT and blow up the retransmit timer.
*/ */
if ((to.to_flags & TOF_TS) != 0 && if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) { to.to_tsecr) {
uint32_t t; uint32_t t;
▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines