Re: CVS commit: src/sys

[Andrew Doran <ad%netbsd.org@localhost>]

Christos,

On Sat, Apr 11, 2009 at 09:35:03PM +0000, Christos Zoulas wrote:

> In article <20090411210833.GO22753%hairylemon.org@localhost>,
> Andrew Doran  <ad%netbsd.org@localhost> wrote:
> >On Sat, Apr 11, 2009 at 11:47:34AM -0400, Christos Zoulas wrote:
> >
> >> Module Name:       src
> >> Committed By:      christos
> >> Date:              Sat Apr 11 15:47:34 UTC 2009
> >> 
> >> Modified Files:
> >>    src/sys/dev/dmover: dmover_io.c
> >>    src/sys/dev/putter: putter.c
> >>    src/sys/kern: kern_drvctl.c sys_mqueue.c
> >>    src/sys/net: bpf.c bpfdesc.h if_tap.c
> >>    src/sys/opencrypto: cryptodev.c
> >>    src/sys/sys: mqueue.h
> >> 
> >> Log Message:
> >> Fix PR/37878 and PR/37550: Provide stat(2) for all devices and don't use
> >> fbadop_stat.
> >
> >The locking is all screwed up or missing. stat() can be expected to report
> >bad times for these files. I wrote the below to you in private and forgot to
> >copy the list.
> >
> 
> I added locking as you suggested, and if it is screwed up or missing
> it is screwed up the same way in all the existing copies of fop_stat()
> which were there before I added mine.

Prior to your change:

soo_stat: good
kqueue_stat: good (single read of a <=uintptr_t sized value is atomic
                and is the only value we are after that can change at
                runtime, so we get an atomic snapshot of everything
                asked for via stat(). no need to synchronize with other
                threads since the caller does not know for sure when
                the stat() will take place in relation to other operations)
vn_statfile: broken, does not lock vnode
pipe_stat: broken, does not lock pipe

> As for invalid timestamps, all set proper timestamps except drvctl and you
> did not mention that.

I was speaking about concurrency and not about other aspects of how
timestamps are managed. I haven't looked at the functional aspects of your
change.

I said that >uintptr_t sized updates are unlikely to be atomic. One example:
if your stat and update race on a 'struct timespec', stat can return a
timestamp that is in the future, because only one chunk of the timespec has
been updated and made globally visible when stat executes.

> It is much easier and saves time to explain what the mistakes are rather
> than sending out messages saying this is wrong or that is wrong.

I did not have the time when I wrote the message to you and I expected that
you would be able to examine the code and see the problems. Anyway:

dmover:

        looks good.

putter:

        +       getnanotime(&pi->pi_atime);
        KERNEL_LOCK(1, NULL);

        the getnanotime needs to be within KERNEL_LOCK for reason outlined
        above.

drvctl:

        looks good.

mqueue:

        mqueue runs without kernel_lock, so we need mq_mtx and not
        kernel_lock for stat. i haven't checked to see if the getnanotime
        calls are within a block where mq_mtx is held (into an existing
        block would of course be best).

bpf_stat:

        looks good.

tap:

        looks good.

cryptodev:

        crypto runs without kernel_lock. a lot of the timespec assignments
        in the diff are done ouside crypto_mtx. on the other side, stat
        needs crypto_mtx and not kernel_lock.