[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: CVS commit: src/sys
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:
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
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:
the getnanotime needs to be within KERNEL_LOCK for reason outlined
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).
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.
Main Index |
Thread Index |