From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47027 invoked by alias); 11 Nov 2016 11:09:37 -0000 Mailing-List: contact newlib-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: newlib-owner@sourceware.org Received: (qmail 47017 invoked by uid 89); 11 Nov 2016 11:09:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.8 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=Hx-languages-length:3677 X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 11 Nov 2016 11:09:26 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5CF2C16; Fri, 11 Nov 2016 03:09:25 -0800 (PST) Received: from [10.2.206.52] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0D65C3F24D for ; Fri, 11 Nov 2016 03:09:24 -0800 (PST) Subject: Re: [PATCH, newlib] Allow locking routine to be retargeted To: newlib@sourceware.org References: <1478792031.1254.1.camel@op.pl> <60acaa10-fb22-8655-fd67-0536fe359871@foss.arm.com> <1478809956.1322.1.camel@gmail.com> From: Thomas Preudhomme Message-ID: Date: Fri, 11 Nov 2016 11:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <1478809956.1322.1.camel@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016/txt/msg01089.txt.bz2 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