On 24/01/22(Mon) 12:06, Klemens Nanni wrote:
> On Sun, Jan 16, 2022 at 09:22:50AM -0300, Martin Pieuchot wrote:
> > IMHO this approach of let's try if it works now and revert if it isn't
> > doesn't help us make progress. I'd be more confident seeing diffs that
> > assert for the right lock in the functions called by uvm_mapanon() and
> > documentation about which lock is protecting which field of the data
> > structures.
>
> I picked `vm_map's `size' as first underdocumented member.
> All accesses to it are protected by vm_map_lock(), either through the
> function itself or implicitly by already requiring the calling function
> to lock the map.
Could we use a vm_map_assert_locked() or something similar that reflect
the exclusive or shared (read) lock comments? I don't trust comments.
It's too easy to miss a lock in a code path.
> So annotate functions using `size' wrt. the required lock.
>
> Index: uvm_map.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.282
> diff -u -p -r1.282 uvm_map.c
> --- uvm_map.c 21 Dec 2021 22:21:32 -0000 1.282
> +++ uvm_map.c 21 Jan 2022 13:03:05 -0000
> @@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
> * Fills in *start_ptr and *end_ptr to be the first and last entry describing
> * the space.
> * If called with prefilled *start_ptr and *end_ptr, they are to be correct.
> + *
> + * map must be at least read-locked.
> */
> int
> uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
> @@ -2206,6 +2208,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
> * If remove_holes, then remove ET_HOLE entries as well.
> * If markfree, entry will be properly marked free, otherwise, no replacement
> * entry will be put in the tree (corrupting the tree).
> + *
> + * map must be locked.
> */
> void
> uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
> @@ -2976,6 +2980,9 @@ uvm_tree_sanity(struct vm_map *map, char
> UVM_ASSERT(map, addr == vm_map_max(map), file, line);
> }
>
> +/*
> + * map must be at least read-locked.
> + */
> void
> uvm_tree_size_chk(struct vm_map *map, char *file, int line)
> {
> Index: uvm_map.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.h,v
> retrieving revision 1.71
> diff -u -p -r1.71 uvm_map.h
> --- uvm_map.h 15 Dec 2021 12:53:53 -0000 1.71
> +++ uvm_map.h 21 Jan 2022 12:51:26 -0000
> @@ -272,7 +272,7 @@ struct vm_map {
>
> struct uvm_map_addr addr; /* [v] Entry tree, by addr */
>
> - vsize_t size; /* virtual size */
> + vsize_t size; /* [v] virtual size */
> int ref_count; /* [a] Reference count */
> int flags; /* flags */
> struct mutex flags_lock; /* flags lock */
>
