On Tue, May 18, 2021 at 01:24:42PM +0200, Martin Pieuchot wrote:
> 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.
>
Also "volatile" tells compiler to not drop or reorder expression due to
optimisation. But this is not such case.
This diff is ok by me.
> > > 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 */
> > >
> > >
>
