Sorry this one fell through the cracks, thanks Artem!

I completed a full stress test (more than a thousand test scenarios) with D59356.188224.patch in 2 days 16 hours.
No problems seen.

In D59356#1382368, @pho wrote:

In D59356#1382342, @sobomax wrote:

In D59356#1381929, @kib wrote:

I think this is fine. I suggest asking Peter to do the full stress2 run with the patch applied.

Hey @pho can you do this? Thanks!

Sure. I have been running tests for a day with D59356.188224.patch added. A full test usually takes 2 1/2 days on my test HW.

In D59356#1382368, @pho wrote:

In D59356#1382342, @sobomax wrote:

In D59356#1381929, @kib wrote:

I think this is fine. I suggest asking Peter to do the full stress2 run with the patch applied.

Hey @pho can you do this? Thanks!

Sure. I have been running tests for a day with D59356.188224.patch added. A full test usually takes 2 1/2 days on my test HW.

In D44456#1379458, @koinec_yahoo.co.jp wrote:

Sorry for delaying updating hmp(4). Currently my university lab is preparing to design a new thread replacement policy model for FreeBSD. I want to reflect that design to hmp(4) in advance so we can avoid major overhaul to the subsystem later. Right now we are preparing workload samples for benchmarks and I will notify you in this thread as soon as the design draft is completed.

Thank you for the update on the situation.

Regarding the "new thread replacement policy model for FreeBSD," are you intending to develop a new scheduler? If so, I look forward to it and would like to cooperate as much as possible.

In D59356#1382342, @sobomax wrote:

In D59356#1381929, @kib wrote:

I think this is fine. I suggest asking Peter to do the full stress2 run with the patch applied.

Hey @pho can you do this? Thanks!

In D59356#1381929, @kib wrote:

I think this is fine. I suggest asking Peter to do the full stress2 run with the patch applied.

I think this is fine. I suggest asking Peter to do the full stress2 run with the patch applied.

@kib thanks, all three addressed in the update. Every restart of the dependency selection now goes through the EJUSTRETURN return to flush_pagedep_deps(), and nothing is kept on the private unfinished list while the softdep lock is dropped for a restart.

@kib updated as requested, thanks

Updated, relative to the previous revision (874f9dd09fea):

Sorry for delaying updating hmp(4). Currently my university lab is preparing to design a new thread replacement policy model for FreeBSD. I want to reflect that design to hmp(4) in advance so we can avoid major overhaul to the subsystem later. Right now we are preparing workload samples for benchmarks and I will notify you in this thread as soon as the design draft is completed.

In D59356#1378765, @sobomax wrote:

In D59356#1378462, @pho wrote:

Nice test scenario!
Reproduced: https://people.freebsd.org/~pho/stress/log/log0694.txt
As mkdir_blkreuse.sh is a shebang script, the file must have the x bit set.

Isn't it already?

Property	Old Value	New Value
File Mode	null	100755

In D59356#1378462, @pho wrote:

Nice test scenario!
Reproduced: https://people.freebsd.org/~pho/stress/log/log0694.txt
As mkdir_blkreuse.sh is a shebang script, the file must have the x bit set.

Nice test scenario!
Reproduced: https://people.freebsd.org/~pho/stress/log/log0694.txt
As mkdir_blkreuse.sh is a shebang script, the file must have the x bit set.

In D59356#1375508, @kib wrote:

In D59356#1375502, @sobomax wrote:

@pho I tried to coerce ChatGPT Codex to write a reproducer, but it keep refusing claiming that's a security concern. I will try maybe to rephrase it as writing a "unit test to validate the fix."

Peter is exceptionally good in creating the test scenarios.

I suggest you to explain the workload in more details, at the low level. I.e. instead of saying 'we install set of packages on memory disk' say something like 'we create UFS on swap-backed(?) md(4) volume of that size, with SU (or SU+J?) enabled, then create that deep file hierarchy and create that many files, each of that size'.

Add a reproducer, worth $30 of Claude Fable credits.

In D44456#1374396, @koinec_yahoo.co.jp wrote:

• Remove code related to getting and setting the capacity value for hmp(4).
  • This code is being removed with the intention of implementing it in hwpstate_intel(4) in the future.
• Create a mapping table between CPU cores and HFI tables, and improve the code to avoid relying on the SMP CPU topology.
  • Due to a misunderstanding of the intel HFI's specifications, I had mistakenly referenced the CPU group from the SMP(4) CPU topology, whereas I should have retrieved the index pointing to the per-core HFI table from the MSR. Sorry.
• Merged the score setting logic for hmp(4) from the changes in https://reviews.freebsd.org/D57940

Based on the status of the discussion at https://reviews.freebsd.org/D56546#1341348, I have first addressed the two issues mentioned above regarding intelhfi(4) itself.
I plan to make further adjustments regarding score configuration and value ranges once the discussion and revision status for hmp(4) have progressed.

Finally, I would like to apologize once again for the significant delays in this work caused by a busy schedule at my day job. While this situation is expected to continue for a while, I intend to proceed with the necessary work little by little.

In D59356#1375502, @sobomax wrote:

@pho I tried to coerce ChatGPT Codex to write a reproducer, but it keep refusing claiming that's a security concern. I will try maybe to rephrase it as writing a "unit test to validate the fix."

@pho I tried to coerce ChatGPT Codex to write a reproducer, but it keep refusing claiming that's a security concern. I will try maybe to rephrase it as writing a "unit test to validate the fix."

• Remove code related to getting and setting the capacity value for hmp(4).
  • This code is being removed with the intention of implementing it in hwpstate_intel(4) in the future.
• Create a mapping table between CPU cores and HFI tables, and improve the code to avoid relying on the SMP CPU topology.
  • Due to a misunderstanding of the intel HFI's specifications, I had mistakenly referenced the CPU group from the SMP(4) CPU topology, whereas I should have retrieved the index pointing to the per-core HFI table from the MSR. Sorry.
• Merged the score setting logic for hmp(4) from the changes in https://reviews.freebsd.org/D57940

• Revert the 'intelhfi' line added to the 'sys/amd64/conf/GENERIC' file. ( I have made revisions based on the comment (https://reviews.freebsd.org/D56546#1341348) received.)

In D58328#1365066, @ziaee wrote:

Thanks for the ping. So I dug in a little further. The existing description is actually aligned with a library, mktemp.3 which has the same description. It can make not only directories, but pipes and fifos and... (etc) which are all normalized to files. Further, mktemp was written by Todd C Miller and even has a website https://www.mktemp.org/ and seems to have the same Nd in every single OS because we're all using the same implementation. So, we should not change it. In fact perhaps it should be in contrib/ to make that more obvious but I do not want to grind that axe.

Thanks for the ping. So I dug in a little further. The existing description is actually aligned with a library, mktemp.3 which has the same description. It can make not only directories, but pipes and fifos and... (etc) which are all normalized to files. Further, mktemp was written by Todd C Miller and even has a website https://www.mktemp.org/ and seems to have the same Nd in every single OS because we're all using the same implementation. So, we should not change it. In fact perhaps it should be in contrib/ to make that more obvious but I do not want to grind that axe.

Ping.

Ping.

Ping.

In D59356#1363271, @pho wrote:

In D59356#1363214, @kib wrote:

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

That would be nice. I have been seeing the panic a few times over the years, but not after Feb 10 2021.

In D59356#1363214, @kib wrote:

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

In D59356#1362297, @sobomax wrote:

In D59356#1362256, @kib wrote:

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

IMO the correct solution for the race in flush_pagedep_deps() as described in the summary is to memoize the vnode returned by get_parent_vp(), and then restart the global loop. When you see the MKDIR_BODY dependency, and the inode number matches the memoized result from get_parent_vp(), you can process it without relocking sd_fslock, which closes the race. If the vp is NULL or does not match the inode number, you vput() it, call get_parent_vp(), and restart the loop.

Thanks, I agree that scanning past the first matching newblk is not appropriate because it changes dependency-processing order.

I will rework flush_pagedep_deps() to retain the vnode returned by get_parent_vp(), reacquire sd_fslock, and restart dependency selection. The MKDIR_BODY operation will only proceed when the newly selected dependency's inode matches the retained vnode, avoiding use of the dap that was selected before get_parent_vp() dropped the lock.

One detail I want to confirm: flush_newblk_dep() also drops and reacquires sd_fslock internally around bwrite() and ffs_update(), then repeats newblk_lookup(). If the D_ALLOCDIRECT completes during one of those operations, the next lookup could expose the retained D_ALLOCINDIR in the same way. Should flush_newblk_dep() return to the outer dependency-selection loop after such an operation, or revalidate the associated MKDIR_BODY dependency after reacquiring sd_fslock?

Rework to preserve dependencies as suggested by @kib

In D59356#1362256, @kib wrote:

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

IMO the correct solution for the race in flush_pagedep_deps() as described in the summary is to memoize the vnode returned by get_parent_vp(), and then restart the global loop. When you see the MKDIR_BODY dependency, and the inode number matches the memoized result from get_parent_vp(), you can process it without relocking sd_fslock, which closes the race. If the vp is NULL or does not match the inode number, you vput() it, call get_parent_vp(), and restart the loop.

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

Some of the facts supporting this diagnosis: