Page MenuHomeFreeBSD

vm_object: Assert that managed pages are on pagequeues when freeing
ClosedPublic

Authored by markj on Fri, Oct 4, 3:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 15, 6:55 AM
Unknown Object (File)
Thu, Oct 10, 4:59 AM
Unknown Object (File)
Sun, Oct 6, 10:40 PM
Unknown Object (File)
Sun, Oct 6, 10:51 AM
Unknown Object (File)
Sun, Oct 6, 4:29 AM
Unknown Object (File)
Sat, Oct 5, 7:24 AM
Unknown Object (File)
Fri, Oct 4, 10:15 PM
Unknown Object (File)
Fri, Oct 4, 10:15 PM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Fri, Oct 4, 3:08 PM
This revision is now accepted and ready to land.Fri, Oct 4, 6:17 PM

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

In D46945#1070223, @kib wrote:

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

But there the page is freed immediately, not during object teardown, where the assertion is added.

In D46945#1070223, @kib wrote:

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

But there the page is freed immediately, not during object teardown, where the assertion is added.

The situation where invalid page was left on the object queue was quite common until recently.

In D46945#1070436, @kib wrote:
In D46945#1070223, @kib wrote:

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

But there the page is freed immediately, not during object teardown, where the assertion is added.

The situation where invalid page was left on the object queue was quite common until recently.

Sure, but even in that case the page must still be added to a page queue, otherwise it is leaked until the VM object is destroyed. The assertion is meant to catch exactly this kind of bug.

In D46945#1070436, @kib wrote:
In D46945#1070223, @kib wrote:

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

But there the page is freed immediately, not during object teardown, where the assertion is added.

The situation where invalid page was left on the object queue was quite common until recently.

Sure, but even in that case the page must still be added to a page queue, otherwise it is leaked until the VM object is destroyed. The assertion is meant to catch exactly this kind of bug.

Yes and yes. But it happens (happen), and the assert is too late to catch the moment.

I do not object against this change, only pointing out my experience.

In D46945#1070648, @kib wrote:
In D46945#1070436, @kib wrote:
In D46945#1070223, @kib wrote:

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

But there the page is freed immediately, not during object teardown, where the assertion is added.

The situation where invalid page was left on the object queue was quite common until recently.

Sure, but even in that case the page must still be added to a page queue, otherwise it is leaked until the VM object is destroyed. The assertion is meant to catch exactly this kind of bug.

Yes and yes. But it happens (happen), and the assert is too late to catch the moment.

I do not object against this change, only pointing out my experience.

I'm less enthusiastic about it. I'm concerned that people, who are not us, will incorrectly conclude that this assertion should hold everywhere.

In D46945#1070681, @alc wrote:
In D46945#1070648, @kib wrote:
In D46945#1070436, @kib wrote:
In D46945#1070223, @kib wrote:

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

But there the page is freed immediately, not during object teardown, where the assertion is added.

The situation where invalid page was left on the object queue was quite common until recently.

Sure, but even in that case the page must still be added to a page queue, otherwise it is leaked until the VM object is destroyed. The assertion is meant to catch exactly this kind of bug.

Yes and yes. But it happens (happen), and the assert is too late to catch the moment.

I do not object against this change, only pointing out my experience.

I'm less enthusiastic about it. I'm concerned that people, who are not us, will incorrectly conclude that this assertion should hold everywhere.

Would a comment help here? I'd really like to have a check of this kind: I've had at least one (probably two) bugs which can leak pages when unwiring them, and the problem only becomes apparent once a significant number of pages disappear, which can be hard to notice. This assertion doesn't catch problems at the source, true, but 1) in the case of the bugs I have in mind, that is impossible, 2) having information about the VM object involved in a leak (which may be spread among many objects and so not apparent in vmstat -o output) is a useful clue.