Page MenuHomeFreeBSD
Differential D20636

amd64 pmap: block on turnstile for lock-less DI
ClosedPublic
Actions

Authored by kib on Jun 14 2019, 12:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 19 2025, 12:47 PM
Unknown Object (File)
Feb 17 2025, 4:21 AM
Unknown Object (File)
Feb 9 2025, 9:32 AM
Unknown Object (File)
Feb 5 2025, 1:02 AM
Unknown Object (File)
Feb 4 2025, 10:32 PM
Unknown Object (File)
Feb 4 2025, 8:20 PM
Unknown Object (File)
Feb 4 2025, 8:19 PM
Unknown Object (File)
Feb 1 2025, 4:35 AM
View All 74 Files
Subscribers
imp

Details

Reviewers
alc
markj
mjg
dougm
Commits
rS349326: amd64 pmap: block on turnstile for lock-less DI.
Summary

Port the code to block on turnstile to lock-less delayed invalidation, instead of yielding. Yield might cause tight loop due to priority inversion.

Since it is impossible to avoid race between block and wake-up, arm 1-tick callout to wakeup when thread blocks itself.

Reported and tested by: mjg

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 24987

Event Timeline

kib created this revision.Jun 14 2019, 12:53 PM
Herald added a subscriber: imp. · View Herald TranscriptJun 14 2019, 12:53 PM
Harbormaster completed remote builds in B24847: Diff 58624.Jun 14 2019, 12:53 PM
alc accepted this revision.Jun 16 2019, 5:17 PM
This revision is now accepted and ready to land.Jun 16 2019, 5:17 PM
markj added inline comments.Jun 16 2019, 7:33 PM
sys/amd64/amd64/pmap.c
1000

Why do you recheck the loop condition here?

alc added a comment.Jun 17 2019, 2:09 AM

I would suggest that you add a comment to the code explaining why the callout is necessary.

kib added a comment.Jun 17 2019, 8:10 AM
In D20636#446616, @alc wrote:

I would suggest that you add a comment to the code explaining why the callout is necessary.

I will do it when I will be back at my workstation (in about a week).

sys/amd64/amd64/pmap.c
1000

I do it to claim that the only race is due to the lost wakeup. We have in the thread finishing DI:

store generation (atomic, works as seq consistent fence)
load pmap_invl_waiters

Without re-check, the order in the waiting thread is

load generation
increment pmap_invl_waiters (seq cst)

which is reverse order to what is needed to make the increment fence to work against the store generation fence. With the re-check, the order becomes

increment pmap_invl_waiters (seq cst)
load generation

You might argue that the callout would close this race anyway, but callout doing the real work means that the thread lost up to 1 tick of the wall time sitting blocked instead of making progress. So leaving callout use only to wakeup race proper is useful.

markj added inline comments.Jun 17 2019, 4:17 PM
sys/amd64/amd64/pmap.c
1000

Thanks. I think this should be explained in a comment.

kib updated this revision to Diff 58906.Jun 23 2019, 9:18 AM

Add comments explaining the callout use and recheck.

This revision now requires review to proceed.Jun 23 2019, 9:18 AM
Harbormaster completed remote builds in B24982: Diff 58906.Jun 23 2019, 9:18 AM
alc added inline comments.Jun 23 2019, 7:48 PM
sys/amd64/amd64/pmap.c
999

should be "page's"

1000

I would say, "... below the current thread's number. Prepare to block so that we do not waste CPU cycles or worse, suffer livelock."

1004

"... impossible to block without ..."

1007

"... incrementing ..."

1014

"... thread's invalidation ..." (Note the spelling correction to "invlidation".)

kib updated this revision to Diff 58921.Jun 23 2019, 7:57 PM
kib marked 5 inline comments as done.

Language corrections.

Harbormaster completed remote builds in B24987: Diff 58921.Jun 23 2019, 7:57 PM
alc added inline comments.Jun 23 2019, 8:31 PM
sys/amd64/amd64/pmap.c
1008

Verb tense: "... which will unblock us if we lose ..."

1014

"Re-check the current ..."

kib updated this revision to Diff 58926.Jun 23 2019, 8:39 PM

Two more language fixes.

Harbormaster completed remote builds in B24992: Diff 58926.Jun 23 2019, 8:39 PM
alc accepted this revision.Jun 23 2019, 8:42 PM
This revision is now accepted and ready to land.Jun 23 2019, 8:42 PM
markj accepted this revision.Jun 23 2019, 8:46 PM
Closed by commit rS349326: amd64 pmap: block on turnstile for lock-less DI. (authored by kib). · Explain WhyJun 23 2019, 9:21 PM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rS349326: amd64 pmap: block on turnstile for lock-less DI..

Revision Contents
Changeset List

PathSize
sys/
amd64/
amd64/
131 lines

Diff 58921

View Options

sys/amd64/amd64/pmap.c

PropertyOld ValueNew Value
svn:eol-stylenullnative
\ No newline at end of property
svn:keywordsnullFreeBSD=%H
\ No newline at end of property
svn:mime-typenulltext/plain
\ No newline at end of property
# $FreeBSD$
PORTNAME= pyfda
PORTVERSION= 0.1
DISTVERSIONSUFFIX= rc6
CATEGORIES= cad python devel
MASTER_SITES= CHEESESHOP
PKGNAMEPREFIX= ${PYTHON_PKGNAMEPREFIX}
MAINTAINER= yuri@FreeBSD.org
COMMENT= GUI tool for designing and analysing discrete time filters
LICENSE= MIT
LICENSE_FILE= ${WRKSRC}/LICENSE
RUN_DEPENDS= ${PYTHON_PKGNAMEPREFIX}docutils>0:textproc/py-docutils@${FLAVOR} \
${PYTHON_PKGNAMEPREFIX}matplotlib>0:math/py-matplotlib@${FLAVOR} \
${PYNUMPY} \
${PYTHON_PKGNAMEPREFIX}scipy>0:science/py-scipy@${FLAVOR}
USES= python:3.4+ pyqt:5
USE_PYTHON= distutils autoplist noflavors
matUnsubmitted
Not Done
Inline Actions

Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is empty, thus, it makes this port broken because the dependencies line end with @ with no flavor.

mat: Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is…
yuriAuthorUnsubmitted
Not Done
Inline Actions

But this is an app, not a library. Shouldn't python apps all be noflavors?

yuri: But this is an app, not a library. Shouldn't python apps all be `noflavors`?
matUnsubmitted
Not Done
Inline Actions

this has a PKGNAMEPREFIX, nothing against it having noflavors.

mat: this has a PKGNAMEPREFIX, nothing against it having noflavors.
USE_PYQT= core_run gui_run widgets_run
NO_ARCH= yes
post-extract:
@${MV} ${WRKSRC}/pyfda/pyfda_log.conf ${WRKSRC}/pyfda/pyfda_log.conf.sample
@${MV} ${WRKSRC}/pyfda/pyfda_log_debug.conf ${WRKSRC}/pyfda/pyfda_log_debug.conf.sample
post-patch:
@${REINPLACE_CMD} -e " \
s|package_data={'pyfda': \['pyfda_log.conf', 'pyfda_log_debug.conf',|package_data={'pyfda': [|; \
s|data_files = \[|data_files = [('${PREFIX}/etc', ['pyfda/pyfda_log.conf.sample', 'pyfda/pyfda_log_debug.conf.sample'])|" \
${WRKSRC}/setup.py
@${REINPLACE_CMD} -e " \
s|logging.config.fileConfig(os.path.join(base_dir, rc.log_config_file))|logging.config.fileConfig(os.path.join('${PREFIX}/etc', rc.log_config_file))|" \
${WRKSRC}/pyfda/pyfdax.py
@${REINPLACE_CMD} -e " \
s|'pyfda.log'|'/tmp/pyfda.log'|" \
${WRKSRC}/pyfda/*.conf.sample
post-stage:
@${REINPLACE_CMD} -E " \
s|(.*\.sample)$$|@sample \1|" \
${WRKDIR}/.PLIST.pymodtmp
.include <bsd.port.mk>