Date: Sat, 28 Jan 2012 18:47:50 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andrey Chernov <ache@freebsd.org> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Bruce Evans <brde@optusnet.com.au>, svn-src-head@freebsd.org, David Schultz <das@freebsd.org>, Mark Murray <markm@freebsd.org> Subject: Re: svn commit: r230230 - head/sys/dev/random Message-ID: <20120128180310.F1114@besplex.bde.org> In-Reply-To: <20120128045854.GA12556@vniz.net> References: <E1Rny2A-000C3x-O6@groundzero.grondar.org> <20120126143819.GA88677@vniz.net> <20120126155626.GA92229@vniz.net> <201201261132.38320.jhb@freebsd.org> <20120126165521.GA92622@vniz.net> <20120126175021.GA93016@vniz.net> <20120127201855.K1604@besplex.bde.org> <20120128043118.GA12241@vniz.net> <20120128045854.GA12556@vniz.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 28 Jan 2012, Andrey Chernov wrote:
> New verson addressed bde's style things:
Thanks, but ...
> --- sys/libkern.h.old 2012-01-16 07:15:12.000000000 +0400
> +++ sys/libkern.h 2012-01-28 08:49:19.000000000 +0400
> @@ -70,6 +70,11 @@ static __inline int abs(int a) { return
> static __inline long labs(long a) { return (a < 0 ? -a : a); }
> static __inline quad_t qabs(quad_t a) { return (a < 0 ? -a : a); }
>
> +#define ARC4_ENTR_NONE 0 /* Don't have entropy yet. */
> +#define ARC4_ENTR_HAVE 1 /* Have entropy. */
> +#define ARC4_ENTR_SEED 2 /* Reseeding. */
> +extern int arc4rand_iniseed_state;
> +
> /* Prototypes for non-quad routines. */
> struct malloc_type;
> uint32_t arc4random(void);
... I will also ask for data and macros to be sorted into the data an
macro sections and not unsorted in between the first set of inlines
and the prototypes. (Whether the macros should be all in a macro
section or near the variables or prototypes that they are for is
unclear, especially since the current organization for this in this
file is random.)
This file was last nearly sorted in FreeBSD-2.1. Now its organization
is partly to put macros and data together with inline functions that
use them. This gives lots of subsections which are not clearly delimited
and is not used in the prototype section, where it would split up that
section too much.
>From your previous reply (non-style part):
> > > +volatile int arc4rand_iniseed_state = ARC4_ENTR_NONE;
> > > +
> >
> > I don't see what all the volatile qualifiers and atomic ops are for.
> > The volatiles certainly have no effect, since this variable is only
> > accessed by atomic ops which only access variables as volatile whether
> > you want this or not. See das's reply for more about the atomic ops.
> > Here I think they just close a window that would be open just once for
> > each for a few instructions while booting.
>
> I will remove volatile.
> About atomic ops: arc4rand calls are protected with mutex and yarrow
> calls are protected, but they can works concurrently wich each
> other. So, if we have atomic ops, why not to use them to close one time
> window too?
Hmm, it's tricky code either way. I use similar cmpsets in sio, and
also need a loop to synchronize the final state change. But when the
state is seen to be final, atomic ops are not needed to check that it
doesn't change (or to see that it is final), since it won't change again.
This gives a tiny optimization that would be larger here. This is mostly
overkill, since sio is^Wwas normally present at boot time. New-bus and
most drivers don't have complications like this, but might need them more
for loadable drivers. It is very unclear that new-bus's Giant locking is
enough for anything (unless the whole system also uses Giant locking).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120128180310.F1114>