[9fans] alt and chanfree

[9fans] alt and chanfree

[Mathieu Lonjaret]

Hi all,

I'm doing something like the following to synchronize a bunch of
coroutines.
Everything goes fine while the different threads are working (and are
regularly sending over their respective channel). However, as soon as
one terminates and calls chanfree(), the next call to alt() fails (as
in I get a "pagefault" message). I can tell because if I never call
chanfree() I never hit that error.
Now in this simplified version I haven't shown that I in fact actually
dynamically reorganize the Alt * (adding/removing an Alt everytime a
caller starts/terminates) so it's totally possible I messed up there.

Before I dig deeper to find out what I'm doing wrong in my Alt *,
I wanted to make sure that I'm globally doing the right thing, i.e. if
it's ok and even recommended to free the Channel as soon as the thread
that was sending on it does not need it anymore.
Can anyone please confirm that?

Thanks,
Mathieu

static void
caller(void *arg)
{
	struct Params{ Torrent *tor; Peer *peer; Channel *c;} *params;
	uchar chanmsg[1];

	params = arg;
	// in callerworks() we send() on params->c at various points to synchronize, all goes well.
	callerworks(params->tor, params->peer, params->c);
	chanmsg[0] = 0;
	send(params->c, chanmsg);
	// peer is done working now
	chanfree(params->c);
}

void
callers(void *arg)
{
	Torrent *tor = arg;
	Peer *peer;
	uchar m[1];
	int i;
	int n;
	Alt *a;
	struct Params{Torrent *tor; Peer *peer; Channel *c;} *params;

	a = emalloc((MAXPEERS+1)*sizeof(Alt));
	for (i = 0; i<MAXPEERS; i++){
		a[i].v = m;
		a[i].c = chancreate(sizeof(m), 0);
		a[i].op = CHANRCV;

		// prepare params for the thread
		peer = emalloc(sizeof(Peer));
		params = emalloc(sizeof(struct Params));
		params->tor = tor;
		params->peer = peer;
		params->c = a[i].c;
		threadcreate(caller, params, STACK);
	}
	a[MAXPEERS].v = nil;
	a[MAXPEERS].c = nil;
	a[MAXPEERS].op = CHANEND;

	for(;;){
		n = alt(a);
		if(n < 0)
			error("with alt");

		if (m[0] == 0){
			// a caller has terminated
			dosomestuff()
		}
	}
}

Re: [9fans] alt and chanfree

[roger peppe]

On 14 September 2010 09:57, Mathieu Lonjaret <mathieu.lonjaret@gmail.com> wrote:
>        for(;;){
>                n = alt(a);
>                if(n < 0)
>                        error("with alt");
>
>                if (m[0] == 0){
>                        // a caller has terminated
>                        dosomestuff()

shouldn't you nil out the appropriate part of the alt
array here? i.e. a[n].c = nil
otherwise you'll be alting on a freed chan which
must be bad.

Re: [9fans] alt and chanfree

[Mathieu Lonjaret]

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

That's exactly the sense of my question, thanks.
I didn't see in the man page if I should do that sort of cleaning up or
not, it just says that the channel can be freed from either side. so I
wasn't sure in what state alt() is expecting the Alt entries to be at
all time.

I'll try it out later today, thanks.

Cheers,
Mathieu

[-- Attachment #2: Type: message/rfc822, Size: 5342 bytes --]

From: roger peppe <rogpeppe@gmail.com>
To: Fans of the OS Plan 9 from Bell Labs <9fans@9fans.net>
Subject: Re: [9fans] alt and chanfree
Date: Tue, 14 Sep 2010 10:55:55 +0100
Message-ID: <AANLkTinXHZ_QbM+CbWwJ_j-t-HY5RtK8Fbwf-i8R+rgr@mail.gmail.com>

On 14 September 2010 09:57, Mathieu Lonjaret <mathieu.lonjaret@gmail.com> wrote:
>        for(;;){
>                n = alt(a);
>                if(n < 0)
>                        error("with alt");
>
>                if (m[0] == 0){
>                        // a caller has terminated
>                        dosomestuff()

shouldn't you nil out the appropriate part of the alt
array here? i.e. a[n].c = nil
otherwise you'll be alting on a freed chan which
must be bad.

Re: [9fans] alt and chanfree

[erik quanstrom]

On Tue Sep 14 06:11:35 EDT 2010, mathieu.lonjaret@gmail.com wrote:

> That's exactly the sense of my question, thanks.
> I didn't see in the man page if I should do that sort of cleaning up or
> not, it just says that the channel can be freed from either side. so I
> wasn't sure in what state alt() is expecting the Alt entries to be at
> all time.

isn't that what chanclose()/chanclosing() is for?

- erik

Re: [9fans] alt and chanfree

[Mathieu Lonjaret]

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

Maybe. Lookman gives me nothing, which page is it please?

Mathieu

[-- Attachment #2: Type: message/rfc822, Size: 3814 bytes --]

From: erik quanstrom <quanstro@labs.coraid.com>
To: 9fans@9fans.net
Subject: Re: [9fans] alt and chanfree
Date: Tue, 14 Sep 2010 09:01:57 -0400
Message-ID: <1c691961f042c7f6194f62781d377b4e@coraid.com>

On Tue Sep 14 06:11:35 EDT 2010, mathieu.lonjaret@gmail.com wrote:

> That's exactly the sense of my question, thanks.
> I didn't see in the man page if I should do that sort of cleaning up or
> not, it just says that the channel can be freed from either side. so I
> wasn't sure in what state alt() is expecting the Alt entries to be at
> all time.

isn't that what chanclose()/chanclosing() is for?

- erik

Re: [9fans] alt and chanfree

[erik quanstrom]

On Tue Sep 14 09:28:48 EDT 2010, mathieu.lonjaret@gmail.com wrote:

> Maybe. Lookman gives me nothing, which page is it please?
>

thread(2).  you may have to update your thread library/man pages.
chanclosing is recent.

- erik

Re: [9fans] alt and chanfree

[Mathieu Lonjaret]

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

Ah, I indeed use an old iso with 9vx, that might be it.

thanks,
Mathieu

[-- Attachment #2: Type: message/rfc822, Size: 3614 bytes --]

From: erik quanstrom <quanstro@quanstro.net>
To: 9fans@9fans.net
Subject: Re: [9fans] alt and chanfree
Date: Tue, 14 Sep 2010 09:30:04 -0400
Message-ID: <df9bb607700278884b89d82be3c63fa6@plug.quanstro.net>

On Tue Sep 14 09:28:48 EDT 2010, mathieu.lonjaret@gmail.com wrote:

> Maybe. Lookman gives me nothing, which page is it please?
>

thread(2).  you may have to update your thread library/man pages.
chanclosing is recent.

- erik

Re: [9fans] alt and chanfree

[Gorka Guardiola]

On Tue, Sep 14, 2010 at 3:27 PM, Mathieu Lonjaret
<mathieu.lonjaret@gmail.com> wrote:
>
> isn't that what chanclose()/chanclosing() is for?
>
> - erik
>
>

Not at all. Chanclose and chanclosing are to be used while the channel
still exists.
A closed channel is not a freed channel. Close/closing are useful for
synchronizing the cleanup
but no other operation should be done on a channel after it has been
freed because
the channel no longer exists.

--
- curiosity sKilled the cat

Re: [9fans] alt and chanfree

[erik quanstrom]

> > isn't that what chanclose()/chanclosing() is for?
> >
> > - erik
> >
> >
>
> Not at all. Chanclose and chanclosing are to be used while the channel
> still exists.
> A closed channel is not a freed channel. Close/closing are useful for
> synchronizing the cleanup
> but no other operation should be done on a channel after it has been
> freed because
> the channel no longer exists.

of course i ment using chanclose as the first step.  the
thread running the alt can free the closed channel.

- erik

Re: [9fans] alt and chanfree

[Mathieu Lonjaret]

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

I haven't tried chanclose() yet, but setting to nil the freed chan in the
alt entry is not really what I wanted since it will make alt() return -1
(better than crashing but not ideal).
In any case I found a satisfying workaround in my algorithm to solve that:
when a thread is done using a chan, I keep track of it instead of calling
chanfree(), and instead of allocating a new chan/alt when creating a new
thread, I simply reuse one of the previously "abandonned" ones (if any
is available). And I'll just free them all when the program terminates.

Thanks to all,
Mathieu

[-- Attachment #2: Type: message/rfc822, Size: 4032 bytes --]

From: erik quanstrom <quanstro@quanstro.net>
To: 9fans@9fans.net
Subject: Re: [9fans] alt and chanfree
Date: Tue, 14 Sep 2010 10:17:13 -0400
Message-ID: <5b73ce333ceed4d8bb71f0e017f70cca@plug.quanstro.net>

> > isn't that what chanclose()/chanclosing() is for?
> >
> > - erik
> >
> >
>
> Not at all. Chanclose and chanclosing are to be used while the channel
> still exists.
> A closed channel is not a freed channel. Close/closing are useful for
> synchronizing the cleanup
> but no other operation should be done on a channel after it has been
> freed because
> the channel no longer exists.

of course i ment using chanclose as the first step.  the
thread running the alt can free the closed channel.

- erik

Re: [9fans] alt and chanfree

[roger peppe]

On 15 September 2010 09:15, Mathieu Lonjaret <mathieu.lonjaret@gmail.com> wrote:
> I haven't tried chanclose() yet, but setting to nil the freed chan in the
> alt entry is not really what I wanted since it will make alt() return -1
> (better than crashing but not ideal).

it wouldn't return -1 (assuming there are at least some valid channels
in the alt array) if you set the op to CHANNOP as well as setting c
to nil.

Re: [9fans] alt and chanfree

[Mathieu Lonjaret]

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

Ah indeed, thank you.

[-- Attachment #2: Type: message/rfc822, Size: 4983 bytes --]

From: roger peppe <rogpeppe@gmail.com>
To: Fans of the OS Plan 9 from Bell Labs <9fans@9fans.net>
Subject: Re: [9fans] alt and chanfree
Date: Wed, 15 Sep 2010 10:47:56 +0100
Message-ID: <AANLkTinE7QC0dUnviKsaJEzT1cxHUxmb+PJ_VxAAJWiC@mail.gmail.com>

On 15 September 2010 09:15, Mathieu Lonjaret <mathieu.lonjaret@gmail.com> wrote:
> I haven't tried chanclose() yet, but setting to nil the freed chan in the
> alt entry is not really what I wanted since it will make alt() return -1
> (better than crashing but not ideal).

it wouldn't return -1 (assuming there are at least some valid channels
in the alt array) if you set the op to CHANNOP as well as setting c
to nil.