This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH roland/lll-futex] Separate Linuxisms from lowlevellock.h, make a generic one


> I'd prefer if the comment would explicitly mention spurious wake-ups;
> what you write could be implementable but is not precisely how we use
> futexes.  Maybe just say that it waits until a real or a spurious
> wake-up occurs?

I made a modicum of effort to document the macros' protocols in the stub
file because it's the generally right thing to do.  But I was just barely
cobbling together rough and terse descriptions of what I wasn't quite sure
things actually did.  I'd love it if you could provide detailed replacement
comments for the stub file that explain the contract the genericish NPTL
source code expects from these macros.  Separately, if there's something
useful to say about how the Linux implementation bridges the gap between
that contract and the kernel's exact contract to put into comments in the
Linux file, I'd appreciate that very much as well.

> Call mutex futexp2 (or something else) instead?  (It's just a parameter
> name, but it's also not one of our mutexes, really...)

I took the old ARM file as the model and didn't change most names.
But I can change these however you think looks best.

> I believe it would be good if we could document these later on; now that
> there's a generic version, there's (more or less) just one place we'd
> need documentation at.

Feel free! :-)

> > +#define __lll_robust_trylock(futex, id) \
> > +  (atomic_compare_and_exchange_val_acq (futex, id, 0) != 0)
> 
> Use atomic_compare_and_exchange_bool_acq (or clean that up later)?

I changed a couple of cases that were val in the old ARM file to be bool
where it was obvious to me that the intended semantics were clearly bool
(which did not affect the generated code for ARM).  This one I was less
sure about, but I guess the semantics of that value test are exactly the
same here as the bool version so I should change it too.

> This differs from __lll_lock (e.g., CAS vs. atomic_exchange).  I suppose
> that resembles the current code we have; nonetheless, I find it
> surprising (e.g., lll_lock will not overwrite a value of 2 in the lock
> (ie, contended), whereas timedlock will).  That changes whether a
> subsequent lll_unlock will do a lll_futex_wake or not.

This change is intended not to materially change the compiled code at all
if possible.  Once we have many more machines consolidated on using a
generic implementation, then you should by all means go to town on
substantive cleanups like this.

> > +/* The states of a lock are:
> > +    0  -  untaken
> > +    1  -  taken by one user
> 
> I guess "(not) acquired" is a better term here?
> 
> > +   >1  -  taken by more users */
> 
> A mutex is never acquired by more than one thread.  IIRC, this state
> means that the lock is acquired by one thread, and other threads _might_
> be trying to acquire the lock as well (including being blocked on the
> futex).

Here I just preserved the existing comments from the old code.  Once my
change lands, please send your own changes to do comment cleanups and the
like.  (A pure comment change like this that is indisputably clarifying
things can go in as "obvious enough" without needing formal review.)


Thanks,
Roland


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]