Date: Thu, 21 May 2009 18:03:12 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jeff Roberson <jroberson@jroberson.net> Cc: =?ISO-8859-15?Q?Dag-Erling_Sm=F8rgrav?= <des@des.no>, jhb@FreeBSD.org, arch@FreeBSD.org Subject: Re: lockless file descriptor lookup Message-ID: <20090521174647.R21310@delplex.bde.org> In-Reply-To: <alpine.BSF.2.00.0905200851390.981@desktop> References: <alpine.BSF.2.00.0905111720280.981@desktop> <86bppy60ti.fsf@ds4.des.no> <alpine.BSF.2.00.0905121411070.981@desktop> <20090514131613.T1224@besplex.bde.org> <alpine.BSF.2.00.0905200851390.981@desktop>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 20 May 2009, Jeff Roberson wrote: > On Thu, 14 May 2009, Bruce Evans wrote: > >> On Tue, 12 May 2009, Jeff Roberson wrote: >> >>> On Tue, 12 May 2009, Dag-Erling Sm?rgrav wrote: >>> >>>> Jeff Roberson <jroberson@jroberson.net> writes: >>>>> I'd also appreciate it if someone could look at my volatile cast and >>>>> make sure I'm actually forcing the compiler to refresh the fd_ofiles >>>>> array here: >>>>> >>>>> + if (fp == ((struct file *volatile*)fdp->fd_ofiles)[fd]) >> >> This has 2 style bugs (missing space after first '*' and missing space >> before second '*'. >> >> It isn't clear whether you want to refresh the fd_ofiles pointer to the >> (first element of) the array, or the fd'th element. It is clear that >> you don't want to refresh the whole array. The above refreshes the >> fd'th element. Strangely, in my tests gcc refreshes the fd'th element >> even without the cast. E.g., > > This is actually intended to catch cases where the descriptor array has > expanded and the pointer to fd_ofiles has changed, or the file has been > closed and the pointer at the fd'th element has changed. I'm attempting to > force the compiler to reload the fd_ofiles array pointer from the fdp > structure. If it has done that, it can not have the fd'th element cached and > so that must be a fresh memory reference. So you want to refresh both (the array element implicitly from the pointer). The above cast is clearly no use for refreshing fdp->fd_ofiles, since its type is that of fdp_ofiles (modulo a '*' or two), while to affect fdp->fd_ofiles it would need to make (at least the fd_ofile part of) (*fdp) volatile, and for that it would need to have the type of fdp (modulo a '*' or two), which is quite different (struct filedesc instead of struct file). It is simplest to make all of (*fdp) volatile. The cast for that is (I think) (volatile struct filedesc *)fdp (normal spelling) or (struct filedesc volatile *)fdp (better spelling). Continued in my reply to jhb's reply (on use of atomic instructions/barriers -- we should be able to drop the volatile cast instead of fixing it as above, but should be more careful about the barriers). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090521174647.R21310>