On 18/05/21(Tue) 12:07, Mark Kettenis wrote:
> > Date: Tue, 18 May 2021 12:02:19 +0200
> > From: Martin Pieuchot <m...@openbsd.org>
> >
> > This allows us to not rely on the KERNEL_LOCK() to check reference
> > counts.
> >
> > Also reduces differences with NetBSD and shrink my upcoming `vmobjlock'
> > diff.
> >
> > ok?
>
> Shouldn't we make ref_count volatile in that case?
I don't know, I couldn't find any evidence about where to use "volatile"
in the kernel.
My understanding is that using "volatile" tells the compiler to not
"cache" the value of such field in a register because it can change at
any time. Is it so?
If that's correct, we should look at any piece of code reading such field
multiple times without using atomic operation, right?
In this case `ref_count' is used once for sanity checks in UVM_MAP_REQ_WRITE()
and after calling atomic_dec_int_nv() in uvm_map_deallocate(). So, I don't
see "volatile" necessary here. Did I miss anything?
There's only a couple of 'volatile' usages in sys/sys. These annotations
do not explicitly indicate which piece of code requires it. Maybe it would
be clearer to use a cast or a macro where necessary. This might help us
understand why and where "volatile" is needed.
> > Index: uvm/uvm_map.c
> > ===================================================================
> > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > retrieving revision 1.274
> > diff -u -p -r1.274 uvm_map.c
> > --- uvm/uvm_map.c 26 Mar 2021 13:40:05 -0000 1.274
> > +++ uvm/uvm_map.c 18 May 2021 09:36:55 -0000
> > @@ -491,12 +491,13 @@ uvm_mapent_addr_remove(struct vm_map *ma
> > /*
> > * uvm_map_reference: add reference to a map
> > *
> > - * XXX check map reference counter lock
> > + * => map need not be locked
> > */
> > -#define uvm_map_reference(_map)
> > \
> > - do { \
> > - map->ref_count++; \
> > - } while (0)
> > +void
> > +uvm_map_reference(struct vm_map *map)
> > +{
> > + atomic_inc_int(&map->ref_count);
> > +}
> >
> > /*
> > * Calculate the dused delta.
> > @@ -4292,7 +4293,7 @@ uvm_map_deallocate(vm_map_t map)
> > int c;
> > struct uvm_map_deadq dead;
> >
> > - c = --map->ref_count;
> > + c = atomic_dec_int_nv(&map->ref_count);
> > if (c > 0) {
> > return;
> > }
> > Index: uvm/uvm_map.h
> > ===================================================================
> > RCS file: /cvs/src/sys/uvm/uvm_map.h,v
> > retrieving revision 1.69
> > diff -u -p -r1.69 uvm_map.h
> > --- uvm/uvm_map.h 12 Mar 2021 14:15:49 -0000 1.69
> > +++ uvm/uvm_map.h 18 May 2021 09:36:36 -0000
> > @@ -259,6 +259,7 @@ RBT_PROTOTYPE(uvm_map_addr, vm_map_entry
> > * read_locks and write_locks are used in lock debugging code.
> > *
> > * Locks used to protect struct members in this file:
> > + * a atomic operations
> > * I immutable after creation or exec(2)
> > * v `vm_map_lock' (this map `lock' or `mtx')
> > */
> > @@ -272,7 +273,7 @@ struct vm_map {
> > struct uvm_map_addr addr; /* [v] Entry tree, by addr */
> >
> > vsize_t size; /* virtual size */
> > - int ref_count; /* Reference count */
> > + int ref_count; /* [a] Reference count */
> > int flags; /* flags */
> > struct mutex flags_lock; /* flags lock */
> > unsigned int timestamp; /* Version number */
> >
> >
