Re: [Xenomai-core] [pipe.c] hairy synchronization -> "flush the output queue upon closure"

[Philippe Gerum <rpm@xenomai.org>]

Dmitry Adamushko wrote:
> Philippe Gerum <rpm@xenomai.org> wrote on 18.11.2005 11:14:26:
> 
>  > > ...
>  > >
>  > > it looks like we can't make the whole xnpipe_release() atomic 
> because of
>  > > PREEMPT_RT + wake_up_interruptible_all() things, right? Or no.
>  >
>  > You must _never_ _ever_ reschedule with the nucleus lock held; this
>  > is a major cause of
>  > jittery I recently stumbled upon that was induced by
>  > xnpipe_read_wait() at that time. So
>  > indeed, xnpipe_release() cannot be made atomic this way under a
>  > fully preemptible kernel.
> 
> Yep.
> 
> Now keeping in mind the observation I have made yesterday, it looks 
> like, in fact, there is no need in wake_up_*(readers) call in 
> file_operations::release(). There is nobody to be woken up at the time 
> when release() is called:
> 
> 1) The reference counter of "file" object is 0, i.e. there are no 
> readers since read() increases a counter before getting blocked.
> 
> 2) noone else can use anew that "file" object since close() does the 
> following:
> 
> filp = files->fd[fd];
> if (!filp)
> goto out_unlock;
> files->fd[fd] = NULL; <--- it's invalid from now on
> 
> so it's not possible that some new readers may occur when a counter == 0.
> 

Ack.

> Hm.. but we still have fasync_helper(-1,file,0,&state->asyncq); which is 
> about sending a signal and that's perfectly valid (a file::counter is 
> not involved here). And that call may lead to re-scheduling (linux 
> re-scheduling of course) so we can't put it in a blocked section.
> 
> So the best way I see is to have something like():
> 
> xnpipe_drop_user_conn()
> {
> xnlock_get_irqsave(&nklock,s);
> 
> while ((holder = getq(&state->outq)) != NULL)
>               
>  state->output_handler(minor,link2mh(holder),-EPIPE,state->cookie);
>            }
> 
>        while ((holder = getq(&state->inq)) != NULL)
>            {
>            if (state->input_handler != NULL)
>               
>  state->input_handler(minor,link2mh(holder),-EPIPE,state->cookie);
>            else if (state->alloc_handler == NULL)
>                xnfree(link2mh(holder));
>            }
> 
> __clrbits(state->status,XNPIPE_USER_CONN);
> 
> xnlock_put_irqrestore(&nklock,s);
> }
> 
> and call it everywhere instead of clrbits(state->status,XNPIPE_USER_CONN);
> 
> This way we may be sure there are no pending messages left.
> 
>

Sounds consistent, since USER_CONN flag is semantically bound to the active/inactive state 
of the associated data queues anyway.

>  > --
>  >
>  > Philippe.
> 
> ---
> 
> Dmitry
> 


-- 

Philippe.