On Fri, Jan 10, 2014 at 08:28:12AM +0100, Marin Ramesa wrote: > On 01/10/2014 01:59:55 AM, Richard Braun wrote: > >On Thu, Jan 09, 2014 at 09:51:51PM +0100, Marin Ramesa wrote: > >> On 01/09/2014 05:52:21 PM, Richard Braun wrote: > >> >On Thu, Jan 09, 2014 at 05:06:09PM +0100, Marin Ramesa wrote: > >> >> Shouldn't the compare be atomic. Maybe I don't understand what > >> >> atomic really > >> >> means but the GCC manual says this function is. Or is it > >enough to > >> >> hold the lock. > >> > > >> >From the GCC manual : > >> >"These built-in functions perform an atomic compare and swap. > >That is, > >> >if the current value of *ptr is oldval, then write newval into > >*ptr". > >> >So tell us what the point of replacing oldval with itself is. > >> > >> I do a swap so it returns true if they are equal. > > > >I'll say it again because it doesn't look like you got it : when I > >point > >something out to you, consider you're wrong, and make the effort to > >question your assumptions. You may end up being right at times, > >but most > >often not. Now please, read your code again, then answer the > >question I > >asked : "So tell us what the point of replacing oldval with itself > >is". > > OK, sorry. There's no point. I couldn't find just an atomic compare, so > I'm using the compare-and-swap this way. I'll read more in the GCC > manual, > maybe there is another way.
Reading correctly aligned integers that are not wider than the processor word size is already atomic. But there still is a problem I mentioned in another review and forgot this time : On Thu, Jan 09, 2014 at 03:45:35PM +0100, Richard Braun wrote: > On Wed, Jan 08, 2014 at 08:43:28PM +0100, Marin Ramesa wrote: > > +routine futex_wait( > > + task : task_t; > > + address : vm_offset_t; > > + value : int; > > + msec : int; > > + private_futex : boolean_t); > > + > > +routine futex_wake( > > + task : task_t; > > + address : vm_offset_t; > > + wake_all: boolean_t); > > Looks good. You'll probably want the requeue operation too some day, but > that can wait. Actually, this isn't good. Rework the futex_wait routine so that it passes *memory* to the kernel, not raw values. Once the kernel can directly access the futex value, a plain memory access should do the job, but since I haven't read the futex paper in depth, I'm not even sure why this is done before queueing a thread on a futex, and what are the exact constraints on the system call. -- Richard Braun