This is the mail archive of the newlib@sourceware.org mailing list for the newlib 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, newlib] Allow locking routine to be retargeted


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


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