From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <49759702.4010903@domain.hid> Date: Tue, 20 Jan 2009 10:18:58 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <496F0DBE.5010108@domain.hid> <496F14B9.4010604@domain.hid> <496F1CF2.1030401@domain.hid> <496F275E.90407@domain.hid> <496F4065.9040501@domain.hid> <496F4558.8080603@domain.hid> <49750A32.8080104@domain.hid> <49750E55.8080000@domain.hid> <4975901E.7040408@domain.hid> <4975942D.60906@domain.hid> In-Reply-To: <4975942D.60906@domain.hid> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] Pending patches 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 Jan Kiszka wrote: > Philippe Gerum wrote: >> Jan Kiszka wrote: >>> Philippe Gerum wrote: >>>> Philippe Gerum wrote: >>>>> Jan Kiszka wrote: >>>>>> Philippe Gerum wrote: >>>>>>> Jan Kiszka wrote: >>>>>>>> Philippe Gerum wrote: >>>>>>>>> Jan Kiszka wrote: >>>>>>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d >>>>>>>>>> Author: Jan Kiszka >>>>>>>>>> Date: Thu Jan 15 11:10:24 2009 +0100 >>>>>>>>>> >>>>>>>>>> xnpipe: Fix racy callback handlers >>>>>>>>>> >>>>>>>>>> Invocation of input, output and alloc handler must take place under >>>>>>>>>> nklock to properly synchronize with xnpipe_disconnect. Change all >>>>>>>>>> callers to comply with this policy. >>>>>>>>>> >>>>>>>>> That one is under investigation. I agree on the bug report (thanks btw), but I >>>>>>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For >>>>>>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux >>>>>>>>> write endpoint. >>>>>>>> You mean it /could/? Because no in-tree user (ie. native) calls >>>>>>>> rt-unsafe services from its alloc_handler. >>>>>>>> >>>>>>> When you export a public interface, it is better not to make it incompatible >>>>>>> unless there is no other way to fix a situation. Doing so is last resort for me. >>>>>> OTH, there is nothing documented yet about those callback handlers or >>>>>> xnpipe_connect. So we could only break _assumptions_ about this >>>>>> interface. >>>>> Actually, we would break existing implementations, but I understand your point. >>>>> >>>>> But, of course, I would be happy if we could continue to keep >>>>>> the critical section length short. I just don't see how to achieve this >>>>>> without significant restrictions on the callback handlers and their use >>>>>> cases. >>>>>> >>>>> That is because the semantics of those callouts is not that smart. If we have to >>>>> break the API to solve the locking issue, I want to get the semantics fixed in >>>>> the same move (which may help a lot in solving the locking issue as well), so we >>>>> don't end up with two subsequent breakage. >>>>> >>>> As suspected, the callback management in the message pipe code had very serious >>>> issues, including the xnpipe_disconnect / xnpipe_release race. At least: >>>> >>>> - the latency would simply skyrocket when the input queue (i.e. userland to >>>> kernel) is flushed from xnpipe_cleanup_user_conn, because the latter holds the >>>> nklock with interrupts off when doing so. I've tested this case on v2.4.x, the >>>> results on a latency test when the pipe test code is running in parallel are not >>>> pretty (i.e. > 220 us latency spot). >>>> >>>> - message buffers waiting in the output queue while the latter is flushed would >>>> NOT be returned to the memory pool when an input handler is present, which is >>>> the case with the native API that provides one. All the confusion went from the >>>> output notification and free buffer logic that were combined into the output >>>> handler. This went unnoticed probably because messages sent via the write() >>>> endpoint are immediately consumed by the receiving side in a RT task that >>>> preempts, but still, the bug is there. >>>> >>>> The disconnect vs release race can be fixed in a (nearly) lockless way using a >>>> lingering close sequence, that prevents xnpipe_disconnect() to wipe out the >>>> extra state (e.g. RT_PIPE struct) until xnpipe_release() does not need it >>>> anymore. In short, if the userland side is connected, xnpipe_disconnect() will >>>> defer the actual release of that extra state to xnpipe_release(); otherwise, it >>>> will discard it immediately. A new callback, namely .release has been added for >>>> that purpose. >>> Just had a short glance so far. Looks good - except that there is still >>> a use (cookie, now xstate, from xnpipe_state) after release >>> (xnpipe_minor_free). Can't we push the minor_free after calling the >>> release handler? >>> >> We should indeed postpone this just in case the upper layer indexes the extra >> state on the minor value. We can also simplify a few things doing so. >> >> --- ksrc/nucleus/pipe.c (revision 4565) >> +++ ksrc/nucleus/pipe.c (working copy) >> @@ -77,11 +77,9 @@ >> >> static inline void xnpipe_minor_free(int minor) >> { >> - if (minor < 0 || minor >= XNPIPE_NDEVS) >> - return; >> - >> - __clrbits(xnpipe_bitmap[minor / BITS_PER_LONG], >> - 1UL << (minor % BITS_PER_LONG)); > > xnarch_write_memory_barrier()? Just for the paranoiacs. Actually, xnarch_memory_barrier(), reading (eg. from xstate) should better be finished by then as well. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux