Page MenuHomeFreeBSD

Remove an overly-aggressive assertion.
ClosedPublic

Authored by jhb on Jun 6 2019, 7:27 PM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Jun 6 2019, 7:27 PM
np accepted this revision.Jun 6 2019, 7:28 PM
This revision is now accepted and ready to land.Jun 6 2019, 7:28 PM
jhb added inline comments.Jun 6 2019, 7:32 PM
sys/vm/vm_map.c
496 ↗(On Diff #58326)

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.

imp accepted this revision.Jun 6 2019, 8:54 PM
markj added inline comments.Jun 6 2019, 8:55 PM
sys/vm/vm_map.c
464 ↗(On Diff #58326)

This comment deserves an update.

jhb added inline comments.Jun 6 2019, 9:35 PM
sys/vm/vm_map.c
464 ↗(On Diff #58326)

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().

alc accepted this revision.Jun 7 2019, 3:37 AM
alc added inline comments.Jun 7 2019, 3:46 AM
sys/vm/vm_map.c
493 ↗(On Diff #58326)

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 accepted this revision.Jun 7 2019, 3:29 PM
markj added inline comments.
sys/vm/vm_map.c
464 ↗(On Diff #58326)

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

jhb updated this revision to Diff 58378.Jun 7 2019, 8:43 PM

Rewrite the comment a bit.

This revision now requires review to proceed.Jun 7 2019, 8:43 PM
alc accepted this revision.Jun 7 2019, 9:40 PM
This revision is now accepted and ready to land.Jun 7 2019, 9:40 PM
jhb added inline comments.Jun 10 2019, 6:56 PM
sys/vm/vm_map.c
493 ↗(On Diff #58326)

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.

jhb updated this revision to Diff 58482.Jun 10 2019, 6:58 PM

Remove banal and potentially misleading comment.

This revision now requires review to proceed.Jun 10 2019, 6:58 PM
Harbormaster completed remote builds in B24798: Diff 58482.
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.
alc added inline comments.Jun 10 2019, 7:41 PM
sys/vm/vm_map.c
493 ↗(On Diff #58326)

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,