vm: Add missing WITNESS warnings for M_WAITOK allocation

[cem]

Summary

_vm_map_clip_end allocates memory M_WAITOK for !system_map vm_maps. Add
WITNESS warning annotation for !system_map callers who may be holding
non-sleepable locks.

The warning in vm_map_delete may be redundant, if the loop is always
entered. I don't know enough about these functions to say whether it is
redundant or not. vm_map_delete is the path through which we actually
encountered the warning.

[markj]

For non-system maps the map lock is a sleepable sx lock. So if a caller calls vm_map_delete(), it must already have acquired that lock, so it can't have been holding a non-sleepable lock. WITNESS already warns about this.

Do you have some modifications which acquire a non-sleepable lock after the map lock?

[cem]

In D25283#557551, @markj wrote:

For non-system maps the map lock is a sleepable sx lock. So if a caller calls vm_map_delete(), it must already have acquired that lock, so it can't have been holding a non-sleepable lock. WITNESS already warns about this.

Do you have some modifications which acquire a non-sleepable lock after the map lock?

Yeah. We have an allocation path which takes a process, drops PROC_LOCK to perform a page-sized allocation, and takes vm_map_lock before taking PROC_LOCK again. If it lost the race to another invocation of the same routine / process, it uses vm_map_delete to free the redundant per-process allocation (before PROC_UNLOCK).

So it seems like the author saw the lock warning, and fixed it, but because they did not encounter the memory sleep warning, they didn't fix that. (And it's somewhat surprising in general that a locked delete/free path sometimes sleeps unbounded, so I have trouble faulting them for that if WITNESS was quiet.)

[markj]

In D25283#557602, @cem wrote:

In D25283#557551, @markj wrote:

For non-system maps the map lock is a sleepable sx lock. So if a caller calls vm_map_delete(), it must already have acquired that lock, so it can't have been holding a non-sleepable lock. WITNESS already warns about this.

Do you have some modifications which acquire a non-sleepable lock after the map lock?

Yeah. We have an allocation path which takes a process, drops PROC_LOCK to perform a page-sized allocation, and takes vm_map_lock before taking PROC_LOCK again. If it lost the race to another invocation of the same routine / process, it uses vm_map_delete to free the redundant per-process allocation (before PROC_UNLOCK).

So it seems like the author saw the lock warning, and fixed it, but because they did not encounter the memory sleep warning, they didn't fix that. (And it's somewhat surprising in general that a locked delete/free path sometimes sleeps unbounded, so I have trouble faulting them for that if WITNESS was quiet.)

I have no objection to the change, but I think we should also warn in vm_map_clip_start() and vm_map_lookup_clip_start(). If you do that, then no modification to vm_map_delete() is needed. Most vm_map_* functions may sleep, basically because they have to interlock with page fault handling, which of course can sleep.

[cem]

In D25283#557899, @markj wrote:

I have no objection to the change, but I think we should also warn in vm_map_clip_start() and vm_map_lookup_clip_start(). If you do that, then no modification to vm_map_delete() is needed. Most vm_map_* functions may sleep, basically because they have to interlock with page fault handling, which of course can sleep.

Sure, I'll do that.

[cem]

Warn in lookup_clip_start and clip_start; drop map_delete warning (inherited from the others).