Page MenuHomeFreeBSD

Remove an overly-aggressive assertion.
ClosedPublic

Authored by jhb on Jun 6 2019, 7:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 19, 7:45 PM
Unknown Object (File)
Tue, Apr 2, 1:46 PM
Unknown Object (File)
Feb 17 2024, 9:04 AM
Unknown Object (File)
Nov 28 2023, 2:46 PM
Unknown Object (File)
Nov 12 2023, 9:06 AM
Unknown Object (File)
Nov 5 2023, 10:47 AM
Unknown Object (File)
Oct 8 2023, 8:03 AM
Unknown Object (File)
Oct 4 2023, 9:45 AM
Subscribers

Details

Summary

While it is true that the new vmspace passed to vmspace_switch_aio
will always have a valid reference due to the AIO job or the extra
reference on the original vmspace in the worker thread, it is not true
that the old vmspace being switched away from will have more than one
reference.

Specifically, when a process with queued AIO jobs exits, the exit hook
in aio_proc_rundown will only ensure that all of the AIO jobs have
completed or been cancelled. However, the last AIO job might have
completed and woken up the exiting process before the worker thread
servicing that job has switched back to its original vmspace. In that
case, the process might finish exiting dropping its reference to the
vmspace before the worker thread resulting in the worker thread
dropping the last reference.

Reported by: np

Diff Detail

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

Event Timeline

This revision is now accepted and ready to land.Jun 6 2019, 7:28 PM
sys/vm/vm_map.c
496

All of the places that call this function do it without holding any locks, so I think it's safe to do the "full" free here instead of only decrementing the reference count, but it'd be great to confirm that assumption. I believe all the craziness with vmspace reference counts is only needed when acquiring a new reference.

sys/vm/vm_map.c
464

This comment deserves an update.

sys/vm/vm_map.c
464

So I reread the comment when looking at this and I believe it is still accurate as what it is talking about is 'newvm' and the reason for the > 0 invariant on the refcount. That part is still always true, and it still allows us to avoid the more complicated logic in vmspace_acquire_ref().

sys/vm/vm_map.c
493

I think that this comment is actually misleading, and reflects the mistaken assertion that you've removed. Specifically, it suggests that the only thing that will happen to the oldvm is that its refcnt will be decremented. I would recommend that you either revise or remove this comment.

markj added inline comments.
sys/vm/vm_map.c
464

Ah, I didn't read it closely enough. :(

Rewrite the comment a bit.

This revision now requires review to proceed.Jun 7 2019, 8:43 PM
This revision is now accepted and ready to land.Jun 7 2019, 9:40 PM
sys/vm/vm_map.c
493

I think I misread your note the first time. If you are saying the comment above 'vmspace_free(oldvm)' is misleading, I think I see what you are saying and can axe it. It's not really needed at this point anyway since it just restates the code.

Remove banal and potentially misleading comment.

This revision now requires review to proceed.Jun 10 2019, 6:58 PM
jhb marked an inline comment as done.Jun 10 2019, 6:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 10 2019, 7:02 PM
This revision was automatically updated to reflect the committed changes.
sys/vm/vm_map.c
493

I think I misread your note the first time. If you are saying the comment above 'vmspace_free(oldvm)' is misleading, I think I see what you are saying and can axe it.

Yes, that is what I meant,