> Date: Tue, 18 May 2021 13:24:42 +0200
> From: Martin Pieuchot <[email protected]>
>
> On 18/05/21(Tue) 12:07, Mark Kettenis wrote:
> > > Date: Tue, 18 May 2021 12:02:19 +0200
> > > From: Martin Pieuchot <[email protected]>
> > >
> > > 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?
Right. So if you want the access to be atomic, it needs to be
"uncached" and therefore you need to use volatile. Now the atomic
APIs explicitly cast their pointer arguments to volatile, so if you
exclusively through those APIs you don't strictly need the variable
itself to be declared volatile. But I think it still is a good idea
to declare them as such.
> If that's correct, we should look at any piece of code reading such field
> multiple times without using atomic operation, right?
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?
If ref_count isn't marked as volatile, some crazy compiler
optimization might interfere with those sanity checks. The refcount
manipulation itself is safe for the reason given above.
> 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.
There are the READ_ONCE() and WRITE_ONCE() macros. I'm not a big fan
of those (since they add clutter) but they do take care of dependency
ordering issues that exist in the alpha memory model. Must admit that
I only vaguely understand that issue, but I think it involves ordered
access to two atomic variables which doesn't seem to be the case.
On non-alpha systems, READ_ONCE() and WRITE_ONCE() just do a volatile
pointer cast.
> > > 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 */
> > >
> > >
>
