Repository navigation
feat(libkernel): support EV_CLEAR on a kqueue watched by another kqueue - #9575
DavidSamir wants to merge 4 commits into
Conversation
FreeBSD re-evaluates a kqueue's EVFILT_READ note from kqueue_wakeup, which runs whenever the watched kqueue queues a note, so an EV_CLEAR note fires again only after such an activation. The equeue now counts activations, and the EV_CLEAR note fires when the count moves while notes are queued.
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
First run on Linux (the PR is tested on Windows only). Ubuntu 26.04, x86_64, Intel i7-8700, GCC 15.2.0, Release, BUILD_TESTING=ON, head bd82db6 merged --no-commit onto current origin/main (6ef5a1c).
- Build: ok.
ctest -R guest_kqueuewith the PR: passes.- Negative control (revert
Equeue.cpp,Equeue.hppandKqueue.cpptoorigin/main, keep the new test): the test aborts with the real assert
terminate called after throwing an instance of 'std::runtime_error'
what(): kevent: EV_CLEAR on a kqueue descriptor not implemented
- Restoring the PR implementation: passes again.
So the EV_CLEAR path and the activation counter are exercised by ClearedKqueue() on Linux too, and the test genuinely fails without them.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The activation snapshot is not atomic, which can permanently lose a kqueue wakeup. PollClearedKqueue first calls CountTriggeredEvents() and then Activations(), and each call acquires/releases the watched queue's mutex independently. If another thread triggers an event after the count returns 0 but before the activation count is read, this poll stores the new generation in note.seen while leaving note.active false. The next poll sees the same generation and never reports the queued event. AddKqueueKnote has the same split-read window while initializing pending and seen.
Please expose one locked snapshot operation that returns both the triggered count and activation generation, and use it in both places (with timer/filter polling performed under the same lock). Add a concurrency regression test that forces or reliably exercises activation between snapshot components; the current sequential ClearedKqueue() test cannot cover this race.
Please also resolve the unverified overlap warnings before approval: #7653 shares Equeue.cpp, Kqueue.cpp, GuestKqueue.cpp and the debt entry, and #7088 shares Equeue.hpp, Kqueue.cpp, GuestKqueue.cpp and the debt entry. The body discusses #7653/#7088 as conflicting with main but does not record author coordination or a required ordering. #6044 also has an uncomparable GuestKqueue.cpp path because its broad test move exceeds the diff limit; preserve this new test at the moved path if that reorganization lands first. If any of these require sequencing, agree the order with the authors and declare the prerequisite in Depends on.
The hosted eligibility, intake, labels, report and conventions checks pass. The Linux, Windows and relinker product jobs are skipped, so I did not treat them as executed. The author-reported Windows test matrix and the independent Linux run in the discussion are useful evidence, but neither exercises the interleaving above.
harisibrar1
left a comment
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 4a17c71 and FreeBSD releng/11.4 sys/kern/kern_event.c:
kqueue_scancallsfilt_kqueueagain before it reports a note (kern_event.c:1782-1792, 1798).PollClearedKqueue(Kqueue.cpp:204-215) only refreshespendingwhen the activation count has moved, so a note that was polled but not collected keepsactiveand its olddata. Example: outer has an EV_CLEAR user note U, added before the EV_CLEAR watch on inner. Trigger inner note 7 (EV_CLEAR) and U, runkevent(outer, n=1)(returns U), then drain inner. The nextkevent(outer)reports the watch with data 1, where FreeBSD reports nothing becausekq_countis 0. With two inner notes and one drained, it reports data 2 where FreeBSD reports 1. One possible fix: setactive = truewhen the count moves, then on every poll clearactiveifpending == 0, else setdata = pending.kqueue_registerre-evaluates the filter on EV_ADD or EV_ENABLE of an existing enabled knote and queues it if ready (kern_event.c:1435-1438, 1468-1478).ModifyKnotere-armsPollEdgewatches for this (Kqueue.cpp:389) but notPollClearedKqueuenotes. So after the first report, a re-EV_ADD(orEV_DISABLE+EV_ENABLE) of the watch while inner still holds note 7 returns 0 here and 1 on FreeBSD. Settingactive = truethere for an enabledPollClearedKqueuenote, together with the check above, should match FreeBSD.- Could both cases go into
ClearedKqueue(), or into the "kevent on a kqueue" debt line if you would rather leave them for later? - Small point: inner notes 7 and 8 are added without EV_CLEAR, so
Poll(inner)does not empty inner. The test is still valid, but the description's "draining the watched kqueue" step does not really drain it.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
…e snapshot A cleared kqueue watch read the watched queue's triggered count and its activation counter under two separate locks, so an activation between the reads was marked seen without making the watch active, and the wakeup was lost. SnapshotTriggered now returns both under one lock. Every poll of an active watch also rechecks the count, as kqueue_scan reruns filt_kqueue: it drops when the watched queue empties and reports the current count. EV_ADD and EV_ENABLE of an existing watch re-arm it. A separate guest_kqueue_cleared_race test triggers the watched queue from another thread while the watching queue waits.
|
Thank you both, these were real. New commits, no force-push:
Negative controls, all on Windows:
Re-run on the merge (Windows 11, WinLibs GCC 15.2): One note for anyone reproducing: On the overlaps (#7653, #7088, and #6044's move of AI-assisted: yes |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Re-ran on Linux at c842a37 (follow-up to my run at bd82db6). Ubuntu 26.04, x86_64 i7-8700 (6 cores / 12 threads), GCC 15.2.0, Release, head merged --no-commit onto origin/main 7e2ba73.
- With the PR:
guest_kqueueandguest_kqueue_cleared_racepass; the race test 10/10 more runs (about 20 s each). - Control,
Equeue.cpp,Equeue.hppandKqueue.cppfrom bd82db6 (before 6deae6e), tests from the head:guest_kqueueaborts (the newClearedKqueueRechecksTheWatchedCount).guest_kqueue_cleared_racepasses 5/5 as registered, and 3/3 more withtaskset -c 0,1/taskset -c 0-3. It only catches the lost wakeup when pinned to one CPU:taskset -c 0 guest_kqueue_tests cleared-raceaborts 3/3 with the old files and passes 5/5 with the head's.
So on a multi-core Linux machine the race test doesn't distinguish the fix as registered. If you want it to, pinning the test to one CPU (or making the trigger thread run between the count and the activation read) would do it; I didn't try the latter.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
On a multi-core Linux machine the race test passed with the split reads it is meant to catch, and caught them only when the process ran on one CPU. It now pins itself to its first allowed CPU on Linux. Windows already catches the split reads unpinned, and pinning there hides them, so it stays unpinned.
|
Thank you @jotitadev, that's exactly the gap. I left Windows unpinned on purpose. There, the split reads fail the race test 8/8 unpinned, but pinned to one CPU they passed 6/6, so pinning would hide the race. On Windows the fix passes in about 11.4 s, and If you have a moment, could you rerun your control on Linux at this head ( AI-assisted: yes |
There was a problem hiding this comment.
Re-reviewed head 7ef0ff542027fba77853362820b763b452641c02. The two earlier functional findings appear addressed: count and activation generation are read in one lock-held snapshot, and the cleared watch is polled against the current watched count on each pass, including re-add/enable. The new Linux race test pins the test and its trigger thread to one allowed CPU, matching the independent reviewer’s negative-control setup. I found no remaining code defect in this pass. The exact guest_kqueue_tests target builds locally. CTest starts both guest_kqueue and guest_kqueue_cleared_race, but each aborts in HostCpu before assertions because this Xeon E5-1620 v2 lacks LZCNT, BMI1/2, MOVBE, AVX2 and FMA. The Windows runs and split-snapshot negative control described in the comments are author-reported; the independent Linux reviewer has tested earlier heads and demonstrated that the split-read control only fails when pinned. Current eligibility, conventions, labels and report pass; product/relinker jobs are skipped. Please resolve the remaining overlap coordination before merge. #7653 edits the same KernelEqueuePrivate::EditEvent function (timer expiry accounting versus this PR's activation snapshot accounting); the changes look composable but need an agreed carry-forward/order. #7088 adds regular-file handling in Kqueue.cpp and tests in GuestKqueue.cpp; the compared functions are separate, but the overlap bot cannot compare these files against its conflicted base and the debt line is shared. #6044 relocates GuestKqueue.cpp and its registration. I found no author agreement in the latest public threads. Please coordinate with these authors, retain the new tests and guest_kqueue_cleared_race registration at the relocated path if #6044 lands first, and declare a dependency only if the agreed sequence requires one. The PR is 4 ahead / 11 behind current main (clean merge). Please refresh it to satisfy the current-main branch rule. One documentation/conventions correction: the current-base convention run reports a debt-length warning at docs/dev/debt/libkernel.md:203 (1,095 characters). The same entry is 960 characters on current main; this PR expanded it past the 1,000-character threshold. Please shorten the entry and correct the Tested text that describes the warning as pre-existing on main.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
harisibrar1
left a comment
harisibrar1
left a comment
There was a problem hiding this comment.
Rechecked at 7ef0ff5:
- Both points are fixed.
PollClearedKqueuenow rechecks the watched count on every poll and drops the watch at 0 (Kqueue.cpp:204-217), andModifyKnotere-arms it onEV_ADD/EV_ENABLE(Kqueue.cpp:392-394). Traced againstkqueue_scanandkqueue_register: the drained case reports nothing, the one-drained case reports data 1, and re-add/enable report 1 (0 once the watched queue is empty), all as on FreeBSD. - Reading the count and the activations under one
SnapshotTriggeredlock (Equeue.cpp:297-306) also closes the window @MarcZX10 found, andClearedKqueueRechecksTheWatchedCountcovers our examples with notes that really drain. - Non-blocking, +1 to @MarcZX10: the "kevent on a kqueue" debt entry goes from 960 characters on main to 1095 here, so this PR is what introduces that
debt-lengthwarning.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 7ef0ff542027fba77853362820b763b452641c02. The snapshot/generation pair is now read under one lock, the cleared watch rechecks the current watched count, and the re-arm paths cover EV_ADD/EV_ENABLE; the independent FreeBSD-semantics review confirms these changes address the earlier functional findings. I found no new implementation defect in those paths.
There are still repository-rule and integration blockers:
core/libs/prx/libkernel/CMakeLists.txtregistersguest_kqueue_cleared_racewithTIMEOUT 60.CONTRIBUTING.mdrequires tests to finish within 30 seconds and says to shrink/split a slow test instead of raising its timeout. The reported Windows run is about 11.4 seconds, but the current test does 5,000 thread creations and 20,000 registered notes, and the independent Linux report measured about 20 seconds per run on its machine. Please reduce or restructure the race test so it retains a useful negative control with adequate CI headroom, and keep its timeout at or below 30 seconds. Please provide an exact-head Linux run for the newly pinned one-CPU path as well; earlier independent Linux runs were on the preceding head.- The new
kevent on a kqueuedebt entry is 1,095 characters on this head; the corresponding current-main entry is 960.tools/check_conventions.py --base upstream/main --head HEADreports a newdebt-lengthwarning at line 203. Please shorten the entry below the 1,000-character limit and retain the gap/console-verification facts. - The branch is based on
d6eafa54d0464258574fd2f71889754d05a5bc93, 54 commits behind currentupstream/main8b3e39832af85f404c6195418d11b3e1ebd1924d. Please update it and rerun the focused matrix. - #7653 and #7088 remain open, and I still see no agreed resolution with their authors for the shared equeue/kqueue changes. #6044 also moves
GuestKqueue.cppand its registration. Please record how both behavior changes andguest_kqueue_cleared_racewill be preserved through the chosen order.
Checks I ran on this exact head: git diff --check passes; the GuestKqueue.cpp source passes GCC 12.2 C++20 -Wall -Wextra -Werror -fsyntax-only; tools/check_test_registration.py --scripts reports 171 registered scripts. Conventions reports 0 errors and the new 1,095-character debt warning. The prior exact-head local test target built, but CTest aborted during HostCpu initialization before assertions because this host lacks LZCNT/BMI/MOVBE/AVX2/FMA. The author reports Windows suites/negative controls; independent Linux runs cover earlier heads and show why the one-CPU control matters. Hosted product jobs are skipped.
I withdraw my earlier generic intake/queue objection. Given your contribution-count eligibility, Unblocks: none is not an intake blocker here.
|
I’ve noticed that my earlier request to hold this PR based on #5239 was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. Please disregard that request. |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
The implementation looks right to me: the lock-held SnapshotTriggered(), the per-poll recheck of the watched count and the re-arm on EV_ADD/EV_ENABLE match kqueue_scan/filt_kqueue, and the new tests fail without the change. What blocks the merge:
core/libs/prx/libkernel/CMakeLists.txt:1518:guest_kqueue_cleared_racehasTIMEOUT 60, and CONTRIBUTING asks for tests that finish within 30 seconds (shrink or split a slow test instead of raising the timeout). It takes about 20 s on an i7-8700 (5000 thread creations, 20000 registered notes,GuestKqueue.cpp:590-598). Please reduce it while keeping a negative control that still fails with the split reads, and set the timeout to 30 or less.docs/dev/debt/libkernel.md:203: thekevent on a kqueueentry grows from 960 to 1095 characters and triggers thedebt-lengthwarning (limit 1000). Please shorten it, keeping theEV_CLEARand "not measured on a console" facts. The Tested section calls that warning pre-existing onmain; it is new with this entry, so please correct it.- The branch conflicts with
mainincore/libs/tests/GuestKqueue.cppanddocs/dev/debt/libkernel.md. Please rebase and rerunguest_kqueueandguest_kqueue_cleared_race. #7653 is already merged, so itsEditEventchange is onmainnow. #7088 is still open and touches the sameKqueue.cpp/GuestKqueue.cpphunks and the debt entry, so check the merge once it lands.
What
keventthrew forEVFILT_READwithEV_CLEARon a kqueue descriptor, that is, one kqueue watching another. In FreeBSD 11:knote_enqueuecallskqueue_wakeup, which runsKNOTEon the watched kqueue'skq_sel.si_note. Sofilt_kqueue(sys/kern/kern_event.c:364,kn_data = kq_count) is evaluated again every time the watched kqueue queues a note.EV_CLEAR:kqueue_scanclears the watching note when it is retrieved, so it fires again only after the next such activation.kqueue_registerevaluates the filter once, so a watched kqueue that already holds notes fires immediately.How:
Equeue.hpp/Equeue.cpp:KernelEqueuePrivatecounts activations, meaning an event becoming enabled and triggered. They come from a poll result, a timer expiring,TriggerEvent,AddEventof a triggered event, a pending event re-queued after retrieval, andEditEvent(the pathNOTE_TRIGGERandEV_ENABLEtake).Activations()reads the count.EditEventcounts through a scope guard, so this change leaves itsNotifyOneline alone.Kqueue.cpp:EV_CLEARon a kqueue no longer throws.filter.data/owner, so the shared event structure is unchanged.dataset to the queued count. The reset after retrieval clears it.kevent on a kqueuedebt line: the "EV_CLEARthrows" sentence now describes this. Activations are seen when the watching queue polls, so one that is undone before that poll is missed.Tested
Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release. This is CPU-side, so no GPU is involved.
guest_kqueue(core/libs/tests/GuestKqueue.cpp) changes:EV_CLEARthrow now adds and deletes the note successfully.ClearedKqueue():data1 andEV_CLEAR;data2;Results:
guest_kqueueand 28 other equeue, kqueue, select, poll, event-flag and AMPR tests pass: 4 solo runs and 4 parallel (-j4) runs.Requirethat reports its line:EditEventactivation count, it fails at the first poll afterNOTE_TRIGGER.tools/check_conventions.py: 0 errors. Its one warning is a debt entry that was already over 1000 characters onmain.tools/pr_overlap.py --branchplusgit merge-tree. This merges cleanly with fix(libkernel): wake every thread waiting on an equeue or kqueue when an event fires #7492 (which changes the same functions'NotifyOnecalls), fix(libkernel): poll kqueues as kqueue_poll answers #7605, fix(libkernel): retire an EV_ONESHOT note through its delete callback and follow Windows $Deleted paths #7049 and feat(libkernel): implement the unnamed ksem semaphores #9284. fix(libkernel): handle kevent timer re-adds and the AIO, LIO, SENDFILE and PROCDESC filters as measured on a PS5 #7653 and feat(libkernel): support kevent read and write events on regular files #7088 already conflict withmain.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)