x86: Use sx lock for intr_table_lock.
ClosedPublic
Actions

Authored by sepherosa_gmail.com on Sep 5 2016, 7:05 AM.

Details

Reviewers

howard0su_gmail.com
honzhan_microsoft.com
decui_microsoft.com
jhb
imp

Commits

rS305722: x86: Use sx lock for interrupt sources.

Summary

Certain pic_assign_cpu, e.g. msi_assign_cpu can have quite a long call chain. For msi_assign_cpu, mutex makes complex PCI bridge drivers more tricky, e.g. sleep can not be used, etc; it will be too tricky for upcoming Hyper-V PCI bridge driver for PCI pass-through.
It is not used on any hot code path nor non-sleepable context, so sx should have the same effect as mutex.

Test Plan

Tested-by: Dexuan Cui <decui microsoft com> w/ WIP PCI bridge driver for Hyper-V PCI pass-through.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

sepherosa_gmail.com updated this revision to Diff 20051.Sep 5 2016, 7:05 AM

sepherosa_gmail.com retitled this revision from to x86: Use sx lock for intr_table_lock..

sepherosa_gmail.com updated this object.

sepherosa_gmail.com edited the test plan for this revision. (Show Details)

sepherosa_gmail.com added reviewers: jhb, imp, decui_microsoft.com, howard0su_gmail.com, honzhan_microsoft.com.

If no objection comes, it will be committed early next week.

My worry is that this has a negative impact on suspend/resume. (In general suspend/resume can't sleep).) One option perhaps would be to split the lock into a mutex protecting list of PICs (that is what suspend/resume needs) and the intr_table_lock would only protect interrupt_sources[].

In D7784#162182, @jhb wrote:

My worry is that this has a negative impact on suspend/resume. (In general suspend/resume can't sleep).) One option perhaps would be to split the lock into a mutex protecting list of PICs (that is what suspend/resume needs) and the intr_table_lock would only protect interrupt_sources[].

OK, I will take a look into this.

Split intr_table_lock into two. One for intr sources, which is now sx. One for PIC, which stays mutex. Suggested-by: jhb

jhb accepted this revision.Sep 8 2016, 7:36 PM

jhb edited edge metadata.

This revision is now accepted and ready to land.Sep 8 2016, 7:36 PM

Closed by commit rS305722: x86: Use sx lock for interrupt sources. (authored by sephe). · Explain WhySep 12 2016, 4:58 AM

This revision was automatically updated to reflect the committed changes.

ehem_freebsd_m5p.com mentioned this in D38450: kern/intr: replace mutex with shared-exclusive lock.Feb 21 2023, 12:20 AM

Revision Contents
Changeset List

Path

Size

sys/

x86/

intr_machdep.c

43 lines

Diff 20051

View Options

sys/x86/x86/intr_machdep.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

x86: Use sx lock for intr_table_lock.ClosedPublicActions