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 <jan.kis...@siemens.com> >>>>>>>>>> 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 _______________________________________________ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core