Re: [RFC] kern: simple futex for gnumach (version 11)

[Richard Braun]

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