From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <48B3BE60.4020709@domain.hid> Date: Tue, 26 Aug 2008 10:27:12 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <48B313B5.9050308@domain.hid> <48B32E0C.7000105@domain.hid> <48B33926.9060808@domain.hid> <48B3B999.6000707@domain.hid> In-Reply-To: <48B3B999.6000707@domain.hid> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] xnregistry_fetch & friends List-Id: "Xenomai life and development \(bug reports, patches, discussions\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: rpm@xenomai.org Cc: xenomai-core Philippe Gerum wrote: > Jan Kiszka wrote: >> Philippe Gerum wrote: >>> Jan Kiszka wrote: >>>> But then I wonder >>>> >>>> a) why xnregistry_fetch uses nklock at all (even for totally uncritical >>>> XNOBJECT_SELF!) >>>> >>> registry_validate() returns a pointer we want to dereference; we'd better keep >>> this unpreemptable, although it's useless for the self-fetching op (which is an >>> unused calling mode so far). If using xnregistry_remove() while fetching the >>> object, the worst case is that your action ends up acting upon an object of the >>> same type, instead of the initially intended one. If that's a problem, goto safe; >> I still don't see the benefit in picking up the object pointer under >> nklock compared to the overhead of acquiring and releasing that lock. >> That's all not truly safe anyway. Even if you ensure that handle and >> object match, someone may change that pair before we can do the lookup >> under nklock. > > Indeed, this is the whole point of the discussion unless I'm mistaken. > >> In my understanding, registry slots either contain NULL or a pointer to >> some object. If that object is valid, that is checked _after_ releasing >> the lock, so we are unsafe again, _nothing_ gained. > > Yes, I agree. I was focusing on _put/_get which maintain the refcount and ought > to be locked, but solely fetching under lock makes no sense. It's probably a > paranoid surge about having dynamically allocated slots, which won't happen > anytime soon. Then you are fine with this patch? --- ChangeLog | 5 +++++ ksrc/nucleus/registry.c | 20 ++++---------------- 2 files changed, 9 insertions(+), 16 deletions(-) Index: b/ChangeLog =================================================================== --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2008-08-26 Jan Kiszka + + * ksrc/nucleus/registry.c (xnregistry_fetch): Remove pointless + locking. + 2008-08-25 Jan Kiszka * include/asm-generic/wrappers.h: Provide atomic_long wrappings. Index: b/ksrc/nucleus/registry.c =================================================================== --- a/ksrc/nucleus/registry.c +++ b/ksrc/nucleus/registry.c @@ -1150,28 +1150,16 @@ u_long xnregistry_put(xnhandle_t handle) void *xnregistry_fetch(xnhandle_t handle) { xnobject_t *object; - void *objaddr; - spl_t s; - xnlock_get_irqsave(&nklock, s); - - if (handle == XNOBJECT_SELF) { - objaddr = xnpod_primary_p()? xnpod_current_thread() : NULL; - goto unlock_and_exit; - } + if (handle == XNOBJECT_SELF) + return xnpod_primary_p()? xnpod_current_thread() : NULL; object = registry_validate(handle); if (object) - objaddr = object->objaddr; + return object->objaddr; else - objaddr = NULL; - - unlock_and_exit: - - xnlock_put_irqrestore(&nklock, s); - - return objaddr; + return NULL; } /*@}*/