[PATCH, newlib] Allow locking routine to be retargeted

[Thomas Preudhomme]

On 10/11/16 20:32, Freddie Chopin wrote:
> Hello again!
>
> On Thu, 2016-11-10 at 16:04 +0000, Thomas Preudhomme wrote:
>>> Why don't you do the same thing for recursive functions? At least
>>> the
>>> lock used by malloc() has to be recursive, so with your patch
>>> exactly
>>> all mutexes should be recursive too.
>>
>> Yes, I did not want to expose all functions until there is a need to
>> differentiate. it is easy to add them later but removing it if noone
>> use it
>> would be quite difficult.
>
> But there is a need to differentiate! Recursive lock is used 10 times
> in newlib (for example in malloc() / free()), non-recursive is used 4
> times (for example for locking time-zone info). If you only allow
> overriding the "standard" functions, you effectively force the user to
> use recursive locks everywhere. I'd definitely be "for" providing
> functions for both lock types!

My idea was indeed that all lock would have to be recursive in a first time and 
differential later if that proves necessary. I'm happy to revisit that though.

>
>>> I have doubts about practical implementation of these functions for
>>> any
>>> RTOS, because of the __LOCK_INIT() macro used for initialization.
>>> In
>>> every retargeted function you'll have to start critical section
>>> (most
>>> likely by disabling interrupts) to initialize the object on heap,
>>> but
>>> then how would you use heap if malloc()'s lock is used via these
>>> functions too?
>>
>> I do not understand. Why would __LOCK_INIT need to start a critical
>> section?
>
> OK, this was not very clear. The problem with simple functions like the
> ones in your patch is that they are extremely hard to actually use in
> an RTOS. Or maybe it just seems so hard for me, that's also possible.
>
> This problem affects only "static" locks created with __LOCK_INIT and
> __LOCK_INIT_RECURSIVE macros. With your patch such locks are just
> uintptr_t with value 0. From my understanding it seems that such locks
> are considered to be "initialized". So you can call __lock_acquire() on
> such object right away. But how would such function look like?
>
> void __lock_acquire(_LOCK_T lock)
> {
> 	if (lock == 0)
> 	{
> 		// lock not yet really initialized...
> 		disableInterrupts();
> 		if (lock == 0)	// re-check due to possible race condition
> 			lock = malloc(sizeof(RealMutexFromYourRtos));
>
> 		assert(lock != 0);
> 		rtos_mutex_create(lock);
> 		enableInterrupts();
> 	}
>
> 	rtos_mutex_lock((RealMutexFromYourRtos*)lock);
> }

Oh I see, that's very clear now, thank you.

>
> This could work, if only malloc() was not protected with the statically
> created _LOCK_T lock itself, which would need such initialization
> too... I don't see any other way to write such function for an RTOS
> which has mutexes with size greater than "uintptr_t" (probably huge
> majority). You could do it with a static pool of storage for mutexes,
> but then you'd have to know upfront how many you want and your RTOS
> would have to allow creation of locks in provided storage (most of them
> allow that, but not all). Maybe I just don't see the simplest solution
> to this problem?

Why do mutex needs to be more than a single integer? How big are mutex in RTOS 
typically in your experience?

>
> Don't get me wrong - I'd really like something like this to be in
> newlib. It just seems that this is extremely hard due to the
> __LOCK_INIT macro which hardcodes the actual size of storage... I'm
> really interested in that feature, as I'd like to provide better newlib
> integration for the RTOS I'm writing ( http://distortos.org/ ).
>
> Regasrds,
> FCh
>

Best regards,

Thomas