Further optimize vm_object_madvise()

[markj]

Summary

Implement the suggestion in D9098 to avoid radix tree lookups of
consecutive pages in the range when a shadow chain is present.

This is based on what vm_object_unwire() does, but we have to handle
the fact that pages may be non-resident at every level of the shadow
chain, and we have to free swap space when applying MADV_FREE.

[markj]

sys/vm/vm_object.c

1125 ↗

(On Diff #24293)

Unrelated to the change, but I'm confused about why this is an "else if".

[markj]

• Use the correct pindex when freeing swap space at the end of the loop.
• Use a helper function for checking whether advice can be applied to an object, and use it to avoid repeating the same check on the top-level object upon each iteration.
• Refactor the loop so that it more closely resembles vm_object_unwire().

[kib]

sys/vm/vm_object.c

1084 ↗

(On Diff #24305)

I like this helper, but I would structure it somewhat different, taking advantage of it being a function with return path:

   if ((object->flags & OBJ_UNMANAGED) != 0)
	   return (false);
   if (advice != MADV_FREE)
            return (true);
   if ((object->type != OBJT_DEFAULT && object->type != OBJT_SWAP) ||
       (object->flags & OBJ_ONEMAPPING) == 0)
	    return (false);
   return (true);

1154 ↗

(On Diff #24305)

Might be extract the conditional call to _freespace() into a helper, as well ? It is repeated three times.

1165 ↗

(On Diff #24305)

I would write an assertion there

MPASS(m == NULL || m->object != object);

if only to help reader.

[markj]

sys/vm/vm_object.c

1084 ↗

(On Diff #24305)

Will do. Regarding OBJ_UNMANAGED, I suspect we want to apply MADV_FREE even to unmanaged pages since it has the effect of clearing the (MI) page dirty bits, while MADV_DONTNEED and MADV_WILLNEED are NOPs for unmanaged pages. That is, I think the "else if" in the original code is intentional.

1154 ↗

(On Diff #24305)

Will do.

1165 ↗

(On Diff #24305)

Hm, I don't think that condition generally holds. Suppose "object" has a backing object and has pages resident at indices N and N+2. After processing the page at N, "m" will be the page at N+2, so pindex < m->pindex and we thus need to search the shadow chain for a page at N+1. But m != NULL && m->object == object.

In fact, a non-NULL "m" always points to a page in the top-level object. Did I misunderstand your suggestion?

[kib]

sys/vm/vm_object.c

1165 ↗

(On Diff #24305)

You are right. Might intent there is to assert that we do not do swap_pager_freespace() for a page for which we did not waited for the busy state. I think I should have used tm instead of m, but then it needs to be initialized to be NULL before the internal loop entrance.

[markj]

Address review comments:

• make vm_object_advice_applies() more succinct
• add a helper for calling swap_pager_freespace()
• add an assertion to the inner loop

[kib]

Are you aware of any code which uses dirty value for unmanaged pages ? For unmanaged pages, we cannot observe mappings, which prevents getting useful values for dirty. In other words, I do not see how could we apply any meaning for MADV_FREE op on unmanaged kind of objects AKA sg/device/phys.

Other than that, I think that the patch is fine.

[markj]

In D9282#192295, @kib wrote:

Are you aware of any code which uses dirty value for unmanaged pages ? For unmanaged pages, we cannot observe mappings, which prevents getting useful values for dirty. In other words, I do not see how could we apply any meaning for MADV_FREE op on unmanaged kind of objects AKA sg/device/phys.

Indeed, I was just thinking it was conceivable that object owners might call vm_page_dirty() directly. However, I can't think of any reason to do this, and I don't know of any examples.

[markj]

Never apply advice to unmanaged pages.

[alc]

sys/vm/vm_object.c

1077–1078 ↗

(On Diff #24410)

I'd suggest writing a comment at the head of this function describing the conditions where advice applies in English.

[alc]

sys/vm/vm_object.c

1147 ↗

(On Diff #24410)

I find the phrase "a common special case" a bit, um, self-contradictory. :-) I would suggest dropping the word "special".

[markj]

• Describe vm_object_advice_applies() in English
• Avoid "common special case"

[markj]

sys/vm/vm_object.c

1147 ↗

(On Diff #24410)

Hm, another way of saying what I meant is, "a common degenerate case," which doesn't seem self-contradictory to me. But I agree that the "special" in that sentence sounds strange and doesn't add anything.

[alc]

sys/vm/vm_object.c

1164 ↗

(On Diff #24410)

I don't see how "tm" is other than NULL here.

[alc]

Mark, aside from the issue that I raised with the assertion, this change looks ready to commit. I just want to say, "Thanks," for following up on my earlier suggestion.

[markj]

Remove an always-true assertion