Re: [Xenomai-core] Pending patches

[Philippe Gerum <rpm@xenomai.org>]

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.kiszka@domain.hid>
>>>>>>>>>> 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.
>

Yes, we should be that paranoid.

>> +	/* May be called with nklock free. */
>> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> +		1UL << (minor % BITS_PER_LONG));
>>  }
>>
>>  static inline void xnpipe_enqueue_wait(struct xnpipe_state *state, int mask)
>> @@ -413,9 +411,9 @@
>>  		__setbits(state->status, XNPIPE_KERN_LCLOSE);
>>  		xnlock_put_irqrestore(&nklock, s);
>>  	} else {
>> -		xnpipe_minor_free(minor);
>>  		xnlock_put_irqrestore(&nklock, s);
>>  		state->ops.release(state->xstate);
>> +		xnpipe_minor_free(minor);
>>  	}
>>
>>  	if (need_sched)
>> @@ -621,9 +619,9 @@
>>  		__clrbits((__state)->status, XNPIPE_USER_CONN);		\
>>  		if (testbits((__state)->status, XNPIPE_KERN_LCLOSE)) {	\
>>  			clrbits((__state)->status, XNPIPE_KERN_LCLOSE);	\
>> -			xnpipe_minor_free(xnminor_from_state(__state));	\
>>  			xnlock_put_irqrestore(&nklock, (__s));		\
>>  			(__state)->ops.release((__state)->xstate);	\
>> +			xnpipe_minor_free(xnminor_from_state(__state));	\
>>  			xnlock_get_irqsave(&nklock, (__s));		\
>>  		}							\
>>  	} while(0)
>>
> 
> Jan
> 


-- 
Philippe.