Re: [PATCH 09/14] Add a new pthread_mutexattr_setelision_np interface.

["Carlos O'Donell" <carlos at redhat dot com>]

On 06/30/2013 05:53 PM, Andi Kleen wrote:
>> Roland objects to this patch principally because the API should
>> be about behaviour guarantees and performance characteristics
>> that the application can describe.
> 
> Elision has a firm abstract behavior model (much firmer than
> most performance critical functions in glibc imho)
> I describe it a bit in the manual, but can expand it if it helps.

It's good and useful to have a firm abstract model for elision,
but that doesn't mean it's part of a useful interface for a user.

Existing glibc functions being poorly designed does not excuse
us from trying to do better.

>> The fact is that elision is an
>> implementation detail of the underlying lock type. The library
>> should be deciding amongst the implementation details given
>> information it knows about the lock.
> 
> The library already does that (that is what the "adaptive elision
> algorithm" is all about). But we don't have enough experience with
> the algorithm yet to be sure that it works well in all cases.

I agree that the library is currently doing this, but with the
new API the user would have specific control over the implementation
choices and that's Roland objects to. The API is exposing internals
that should not be exposed, and that users don't know how to use.

> Also for experimenting with the algorithms there needs to be some
> way to turn it on/off per lock. That is what the interface does.

In which case what you're asking for is an API to be added forever
to glibc for a potentially temporary problem. That's fine, but we've
never done anything like that in the history of the project, so it's
going to take some discussion if we chose to go that route.

It's probably easier to use the generic tunnables inteface I'm
trying to add to do the kind of experimenting your interested in, 
but that's another interface to design.

>> What API would allow us to describe the lock in such a way that
>> glibc could make the decision to use or not use elision?
> 
> Well in theory you could describe abort reasons one by one
> (may do syscalls all the time, may conflict all the time etc.)
> And say do not elide if you know that you would abort for this bit.
> But TSX does not guarantee aborts in all these cases, that 
> is just what the current implementation does. And POWER, zSeries
> etc. may do something different.

That's fine, but the lock hints or critical section hints are always
valid and provide the implementation more information about the lock
thus allowing it to make an informed choice. It's up to the machine
maintainers to do useful things with the hints.

> Could be done, but would be likely awkward.

Could you describe exactly what you mean by awkward?

I'm thinking of:

* Describe the average critical section for the lock:
- Does I/O? (yes/no)
- Size of critical section? (insn count)
- Syscalls (yes/no)
- How much data is accessed (bytes)
- ...
* Lock hints recorded in a table.
* When lock is initialized the library uses the hints
  to choose an implementation.

Cons: Precludes static locks.
Pros: Precise description of the average critical section
      of the lock allows the library to select amongst
      the most useful implementations for the lock.      

> Also I looked through existing glibc nterfaces 
> for precedence, and as far as I can see this interface is doing exactly
> the same as mallopt() does:
> - pure tuning hint
> - no not undefined program should be affected
> - allows to tune some internals
> - it's all hint, if the internal implementation changes it just ignores
> the hint.

If anything I want to get away from specific interfaces for
tunning and move to a unified tunnables interface. I would
not want to see a variant of mallopt() for threading e.g.
threadopt().

I would rather see a generic tunnables interface we can use
to tune all parts of the runtime.

Again, these are my opinions, and they still need vetting and
discussion with other developers in the community that may have
experience to draw upon for the implementation.

> I presume we could define an indexed name space like mallopt() 
> if that helps?

It doesn't help, because mallopt is still not a good solution.

The user still needs to know a-priori if they want elision,
and thus they need to know a lot about the HTM they are
running on.

The suggestion is to describe the critical section and let the
library decide if elision should or should not happen.

> Can you please ask Roland to explain why mallopt() is ok and 
> pthread_{rwlock,mutex}attr_setelision_np() is not and what the
> differences are?

I don't need to ask Roland.

The fact that there is prior art does not give one license
to do the same thing again. We made mistakes in the past and
we are trying not to make the same mistakes in the future.

The mallopt interface is a direct interface into the malloc
implementation. It shouldn't exist. Instead we should have
a very easy to use pluggable mechanism to add alternate malloc
implementations. While you can currently plugin an alternate
malloc, it's incredibly hard to do so correctly, and we don't
help. Thus users wanted knobs and over the years they got them.
The fixed knobs make malloc very difficult maintain.

I would expect mallopt to be subsumed into the generic tunables
interface where we explicitly exclude the flags from the ABI
and make little to no guarantees.

>> I will review this patch anyway because I feel that the review
>> of the patch is instructive and because this patch could get
>> picked up by a downstream that desires to add these interfaces
>> and maintain the as an experimental interface.
> 
> That would fragment the glibc ABI. Is that what you're suggesting?

Downstream can do whatever it wants and make whatever guarantees
it wants. I don't want to judge them.

A fragmented glibc ABI is no different from running a kernel with
or without support for certain features.

I think a fragmented glibc ABI is not a good thing, but for
experimentation purposes I see no other way than to transiently
fragment the ABI and get testing feedback from say Fedora Rawhide.

That's something else that the community has never talked about
e.g. how do we enable experimental API's for developer and user
testing.

>>>      __pthread_mutexattr_settype;
>>> +    pthread_mutexattr_setelision_np;
>>
>> This doesn't look like the nptl/Version file I have on master,
>> so I figure you have some additional changes that should be
>> reverted.
> 
> This is relative to the earlier NORMAL patch, which adds new symbols there.
> 
> I'll send a new patch tomorrow with the update you asked for.

Thanks.

>>
>> You don't need this? 
>>
>> If you add the interfaces, then you add their support to the testsuite.
> 
> I don't have the capability to maintain that many versions
> of the test suite files.  I'll remove all the tests related to this.

OK.

Cheers,
Carlos.