SMP question w.r.t. reading kernel variables

[John Baldwin]

On Monday, April 18, 2011 4:22:37 pm Rick Macklem wrote:
> > On Sunday, April 17, 2011 3:49:48 pm Rick Macklem wrote:
> > > Hi,
> > >
> > > I should know the answer to this, but... When reading a global
> > > kernel
> > > variable, where its modifications are protected by a mutex, is it
> > > necessary to get the mutex lock to just read its value?
> > >
> > > For example:
> > > A if ((mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0)
> > >           return (EPERM);
> > > versus
> > > B MNT_ILOCK(mp);
> > >      if ((mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0) {
> > >           MNT_IUNLOCK(mp);
> > >           return (EPERM);
> > >      }
> > >      MNT_IUNLOCK(mp);
> > >
> > > My hunch is that B is necessary if you need an up-to-date value
> > > for the variable (mp->mnt_kern_flag in this case).
> > >
> > > Is that correct?
> > 
> > You already have good followups from Attilio and Kostik, but one thing
> > to keep
> > in mind is that if a simple read is part of a larger "atomic
> > operation" then
> > it may still need a lock. In this case Kostik points out that another
> > lock
> > prevents updates to mnt_kern_flag so that this is safe. However, if
> > not for
> > that you would need to consider the case that another thread sets the
> > flag on
> > the next instruction. Even the B case above might still have that
> > problem
> > since you drop the lock right after checking it and the rest of the
> > function
> > is implicitly assuming the flag is never set perhaps (or it needs to
> > handle
> > the case that the flag might become set in the future while
> > MNT_ILOCK() is
> > dropped).
> > 
> > One way you can make that code handle that race is by holding
> > MNT_ILOCK()
> > around the entire function, but that approach is often only suitable
> > for a
> > simple routine.
> > 
> All of this makes sense. What I was concerned about was memory cache
> consistency and whet (if anything) has to be done to make sure a thread
> doesn't see a stale cached value for the memory location.
> 
> Here's a generic example of what I was thinking of:
> (assume x is a global int and y is a local int on the thread's stack)
> - time proceeds down the screen
> thread X on CPU 0                    thread Y on CPU 1
> x = 0;
>                                      x = 0; /* 0 for x's location in CPU 1's memory cache */
> x = 1;
>                                      y = x;
> --> now, is "y" guaranteed to be 1 or can it get the stale cached 0 value?
>     if not, what needs to be done to guarantee it?

Well, the bigger problem is getting the CPU and compiler to order the
instructions such that they don't execute out of order, etc.  Because of that,
even if your code has 'x = 0; x = 1;' as adjacent threads in thread X,
the 'x = 1' may actually execute a good bit after the 'y = x' on CPU 1.
Locks force that to sychronize as the CPUs coordinate around the lock cookie
(e.g. the 'mtx_lock' member of 'struct mutex').

> Also, I see cases of:
>      mtx_lock(&np);
>      np->n_attrstamp = 0;
>      mtx_unlock(&np);
> in the regular NFS client. Why is the assignment mutex locked? (I had assumed
> it was related to the above memory caching issue, but now I'm not so sure.)

In general I think writes to data that are protected by locks should always be
protected by locks.  In some cases you may be able to read data using "weaker"
locking (where "no locking" can be a form of weaker locking, but also a
read/shared lock is weak, and if a variable is protected by multiple locks,
then any singe lock is weak, but sufficient for reading while all of the
associated locks must be held for writing) than writing, but writing generally
requires "full" locking (write locks, etc.).

The case above may be excessive caution on my part, but I'd rather be safe than
sorry for writes.

-- 
John Baldwin