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>
index | next in thread | previous in thread | raw e-mail
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). Brucehome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090521174647.R21310>