[Xenomai-core] nucleus/pipe.c patch: check for lingering close at xnpipe_connect()

[Xenomai-core] nucleus/pipe.c patch: check for lingering close at xnpipe_connect()

[Andreas Glatz]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

Hi,

Currently, you can destroy RT_PIPE while a non-rt application is
connected. If that happens the pipe won't be fully closed but
some parts will be leftover for cleanup when closing the RT_PIPE
from the non-rt application (lingering close).

For us it would be very convenient if we didn't have to close
the non-rt side before restarting the rt-side (currently, the 
rt-side complains bc the non-rt side didn't resolve the lingering
close).

Here is a patch where the rt-side resolves the lingering close
when connecting to the RT_PIPE. I tested it and it seems to work 
fine. Do you have any concerns?

Andreas



[-- Attachment #2: lclose.patch --]
[-- Type: text/x-patch, Size: 1028 bytes --]

--- pipe.orig.c	2009-09-09 17:23:10.000000000 -0400
+++ pipe.c	2009-09-09 17:32:48.000000000 -0400
@@ -309,17 +309,35 @@
 int xnpipe_connect(int minor, struct xnpipe_operations *ops, void *xstate)
 {
 	struct xnpipe_state *state;
-	int need_sched = 0, ret;
+	int need_sched = 0, test_lclose = 0, ret;
 	spl_t s;
 
-	minor = xnpipe_minor_alloc(minor);
-	if (minor < 0)
+	ret = xnpipe_minor_alloc(minor);
+	if (ret >= 0)
+		minor = ret;
+	else 
+	if(ret == -EBUSY)
+		test_lclose = 1;
+	else 
 		return minor;
 
 	state = &xnpipe_states[minor];
 
 	xnlock_get_irqsave(&nklock, s);
 
+	if(test_lclose) {
+		if(testbits(state->status, XNPIPE_KERN_LCLOSE)) {
+			clrbits(state->status, XNPIPE_KERN_LCLOSE);
+			xnlock_put_irqrestore(&nklock, s);
+			state->ops.release(state->xstate);
+		} else {
+			xnlock_put_irqrestore(&nklock, s);
+			return -EBUSY;
+		}
+	}
+			
+	xnlock_get_irqsave(&nklock, s);
+
 	ret = xnpipe_set_ops(state, ops);
 	if (ret) {
 		xnlock_put_irqrestore(&nklock, s);

Re: [Xenomai-core] nucleus/pipe.c patch: check for lingering close at xnpipe_connect()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]

Andreas Glatz wrote:
> Hi,
> 
> Currently, you can destroy RT_PIPE while a non-rt application is
> connected. If that happens the pipe won't be fully closed but
> some parts will be leftover for cleanup when closing the RT_PIPE
> from the non-rt application (lingering close).
> 
> For us it would be very convenient if we didn't have to close
> the non-rt side before restarting the rt-side (currently, the 
> rt-side complains bc the non-rt side didn't resolve the lingering
> close).
> 
> Here is a patch where the rt-side resolves the lingering close
> when connecting to the RT_PIPE. I tested it and it seems to work 
> fine. Do you have any concerns?
> 
> Andreas
> 

Whenever possible, please post patches inline (I had to manually copy it
to allow commenting).

> --- pipe.orig.c	2009-09-09 17:23:10.000000000 -0400
> +++ pipe.c	2009-09-09 17:32:48.000000000 -0400
> @@ -309,17 +309,35 @@
>  int xnpipe_connect(int minor, struct xnpipe_operations *ops, void *xstate)
>  {
>  	struct xnpipe_state *state;
> -	int need_sched = 0, ret;
> +	int need_sched = 0, test_lclose = 0, ret;
>  	spl_t s;
>  
> -	minor = xnpipe_minor_alloc(minor);
> -	if (minor < 0)
> +	ret = xnpipe_minor_alloc(minor);
> +	if (ret >= 0)
> +		minor = ret;
> +	else 
> +	if(ret == -EBUSY)
> +		test_lclose = 1;
> +	else 
>  		return minor;
>  
>  	state = &xnpipe_states[minor];
>  
>  	xnlock_get_irqsave(&nklock, s);
>  
> +	if(test_lclose) {
> +		if(testbits(state->status, XNPIPE_KERN_LCLOSE)) {
> +			clrbits(state->status, XNPIPE_KERN_LCLOSE);
> +			xnlock_put_irqrestore(&nklock, s);
> +			state->ops.release(state->xstate);

You cannot bluntly call release() here. You may not run in Linux context
while that handler demands this. Moreover, releasing the state is the
job of the NRT user that opened it and obviously still has a hand on it.

(And the locking is imbalanced here, but that is shadowed by the other
flaws.)

> +		} else {
> +			xnlock_put_irqrestore(&nklock, s);
> +			return -EBUSY;
> +		}
> +	}
> +			
> +	xnlock_get_irqsave(&nklock, s);
> +
>  	ret = xnpipe_set_ops(state, ops);
>  	if (ret) {
>  		xnlock_put_irqrestore(&nklock, s);

IIUC, you are looking for a way to keep a pipe open on the NRT side
while the RT user is disappearing and re-appearing? Not sure if this is
feasible at all with current xnpipe. But if it is, I think you rather
have to introduce some kind of reset on the affected xnpipe_state.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] nucleus/pipe.c patch: check for lingering close at xnpipe_connect()

[Andreas Glatz]

Hi,


> 
> Whenever possible, please post patches inline (I had to manually copy it
> to allow commenting).

Sure, no problem.


> 
> You cannot bluntly call release() here. You may not run in Linux context
> while that handler demands this. 

It's also called in xnpipe_disconnect() (if I follow 'goto cleanup' and
the user-space isn't connected). So is there a different policy here?
I mean should xnpipe_connect() just be called from rt context whereas
xnpipe_disconnect() has to be called from non-rt context?


> Moreover, releasing the state is the
> job of the NRT user that opened it and obviously still has a hand on it.

Yeah, that's the tricky part. I left that part out yet ...

> 
> (And the locking is imbalanced here, but that is shadowed by the other
> flaws.)

Agree.

If we had that new pipe behaviour it could significantly 
speed-up debugging for us because we wouldn't have to stop (and
eventually start) all NRT applications before restarting the RT
application.

Thanks for commenting on my quick and dirty patch. I think, now
I know where to continue.

Andreas

Re: [Xenomai-core] nucleus/pipe.c patch: check for lingering close at xnpipe_connect()

[Philippe Gerum]

On Thu, 2009-09-10 at 12:49 -0400, Andreas Glatz wrote:
> Hi,
> 
> 
> > 
> > Whenever possible, please post patches inline (I had to manually copy it
> > to allow commenting).
> 
> Sure, no problem.
> 
> 
> > 
> > You cannot bluntly call release() here. You may not run in Linux context
> > while that handler demands this. 
> 
> It's also called in xnpipe_disconnect() (if I follow 'goto cleanup' and
> the user-space isn't connected). So is there a different policy here?
> I mean should xnpipe_connect() just be called from rt context whereas
> xnpipe_disconnect() has to be called from non-rt context?
> 
> 
> > Moreover, releasing the state is the
> > job of the NRT user that opened it and obviously still has a hand on it.
> 
> Yeah, that's the tricky part. I left that part out yet ...
> 
> > 
> > (And the locking is imbalanced here, but that is shadowed by the other
> > flaws.)
> 
> Agree.
> 
> If we had that new pipe behaviour it could significantly 
> speed-up debugging for us because we wouldn't have to stop (and
> eventually start) all NRT applications before restarting the RT
> application.
> 
> Thanks for commenting on my quick and dirty patch. I think, now
> I know where to continue.
> 

Allowing the RT side to be re-opened multiple times won't fly, I'm
afraid. The reason we have the linger-on-close behavior is to prevent
the NRT side to refer to stale memory whenever the RT side preempts and
disconnects. Excluding RT from preempting the write/read calls could
cause high latencies (NRT may sleep anyway, so locking would not even be
a safe option there).

Maybe you should move the RT_PIPE descriptor in a separate, standalone
module from your driver to keep the connection alive, and ignore the
-EBUSY status upon failed attempt to connect to the same minor from your
driver? That descriptor is shareable in kernel space so there is no
restriction in using it from another module, and this would allow your
driver module to be unloaded without actually tearing down the data
channel with your userland app.

> Andreas
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
-- 
Philippe.