cdev_pager_allocate(): ensure that the cdev_pager_ops ctr is called only once

[kib]

Summary

per allocated vm_object. Otherwise, since constructors are not idempotent, we e.g. leak device reference in case of non-managed pager.

PR: 278826

[markj]

sys/vm/device_pager.c

183

I think you want to at least use vm_object_deallocate(). Otherwise this will run afoul of the assertions in vm_object_zdtor().

208–210

object->handle has already been assigned, no?

220

Do you need to wake up sleepers here too?

[kib]

sys/vm/device_pager.c

183

I do not want to deallocate() it, because terminate() destroys pager. This is why I did this manually.

[kib]

Handle first batch of comments.

[markj]

sys/vm/device_pager.c

183

IMO it would be cleaner to postpone initialization of object1 fields until after the lookup. Then I think object1->type = OBJT_DEAD; vm_object_deallocate(object1); is sufficient.

217

Now we have a situation where vm_pager_object_lookup() can return an object that is not fully constructed. Is that safe?

[kib]

sys/vm/device_pager.c

217

It should be, since dev_pager_object_list is static in device_pager.c, and all lookups there are plugged with the check for un_pager.devp.dev != NULL.

[kib]

Use vm_object_deallocate()

[markj]

sys/vm/device_pager.c

121

Presumably it's possible to have object == NULL here?

[kib]

sys/vm/device_pager.c

121

I checked both in-tree callers (ttm and linux_page.c) and both seems to be fine with sleeping in the function.

[kib]

Check for object == NULL

[markj]

sys/vm/device_pager.c

44

This isn't needed anymore.

[kib]

Remove refcount.h

[alc]

sys/vm/device_pager.c

208

Defer this until we set the color?

[kib]

Use vm_object_deallocate() instead of _terminate() on cleanup path.
Defer OBJ_COLORED setting after ctr provides the color.

[alc]

sys/vm/device_pager.c

168–169

Please consider reducing the scope of object1 to then clause of the if the statement, The only reason to have this assignment here is to satisfy the MPASS after the else clause. The MPASS could instead be at the end of the then clause.

[kib]

Reduce scope for object1 and color variables.
Assign OBJT_DEAD type to object before deallocating it on the path after failing ctr, since we do not want to have the device dtr called.

[alc]

sys/vm/device_pager.c

171

This initialization isn't really needed. The variable is unconditionally assigned to just a few lines below.

[alc]

sys/vm/device_pager.c

196–197

Here's my last question: Do we need to worry about concurrent updates to size? Specifically, two concurrent updaters that are setting it to different values? We might end up in the 'then' clause in both cases, and not necessarily wind up with the larger of the values assigned to size.

[kib]

sys/vm/device_pager.c

196–197

I am not sure about historic reasons, but I might imagine that some driver relies on extending the pager size.

Is it a problem? The reassignment to the object->size are serialized with dev_pager_mtx.

[alc]

sys/vm/device_pager.c

196–197

I overlooked dev_pager_mtx still being held in this case.