tipc-discussion

[Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

Hi all,
I just uploaded a new file release to SourceForge.
I have corrected several bug related to message bundling and
link changeover, plus a very serious lock bug that sometimes
caused my processors to hang when they lost contact with each
other.
I also corrected a bug in the name/network subscription code,
so you will now receive an event for all overlapping, already
existing publications, not only the first one found ;-).
Some left-out symbols were finally exported as they should.

There are still problems, and strange behaviour that puzzles me,
but it is getting stabler by the day. Among the problems I see:

---When I run the benchmark program, there seems to be a much wider
    time lapse between the termination of the first process and the last
    one for a certain message size than I would expect. The benchmark
     runs to the end without problems, but this does not look right, and
     influences the benchmark result.

--- Even apart from the above, performance seems to be poorer than
     in the old version (the 1.2-line) , for no obvious reason. With the 
code
     changes I have done, I would expect about the same performence.
     At least on SMP the figures look bad, despite the fine lock 
granularity
     we have now. This must be investigated. I have not done any comparative
     measurements on the same hardware and same kernel with the two TIPC
      versions, so this is just an impression I have, but the figures 
worry me.
     Anybody  feeling a vocation here ?


Also, I plan to announce TIPC at the LKML by the end of this week. We must
admit that the code is still in beta status, but as I understand it this 
should
be no problem.
In relation to this, I would like to have a better overview of the code 
status.
Has anybody tested the management interface all the way (Mark?)
Has anybody tried the SOCK_STREAM interface yet ?
What is the status of the multicast code ? (there were some discussions
about who should receive the messages last week, -has this been followed up
with corrections) ?
The reliable broadcast code ?
Are there any known bugs/problems that have not been corrected yet.

Regards /jon
 

Re: [Tipc-discussion] New file release tipc-1.3.10

[Daniel M. <da...@os...>]

Jon,

Thanks for the update.  Mark and I have been testing TIPC and been
been hitting a few problems when sending large packets and then
sending to the management report.  If the returned data was larger
than a packet, the machine would reboot.  This was caused by an
incorrect fragment number.  Your latest update fixed this problem.
Mark has couple small patches coming that fixes a couple of other
things we found.

Daniel

Re: [Tipc-discussion] New file release tipc-1.3.10

[Mark H. <ma...@os...>]

On Tue, 2004-04-27 at 16:58, Jon Maloy wrote:
> Hi all,
> I just uploaded a new file release to SourceForge.

> Has anybody tested the management interface all the way (Mark?)

I've been trying it out.  I'm still having trouble in some
circumstances.  Sometimes, while the tipc benchmark program is running,
and I try to get information from the management interface I'll get
stuck waiting in poll.  If I kill the server side while stuck, the
client side will freeze and requires a reboot.  If I kill the program
waiting on the management interface first and then kill the server side
program, I have no problems. I haven't gotten too far into figuring out
what is going on with this one yet but it sounds like a lock problem. 
Especially since it only seems to happen with SMP.


> Has anybody tried the SOCK_STREAM interface yet ?
> What is the status of the multicast code ? (there were some discussions
> about who should receive the messages last week, -has this been followed up
> with corrections) ?
> The reliable broadcast code ?

> Are there any known bugs/problems that have not been corrected yet.

I have enclosed a couple small patches that were found while debugging
access to the management interface while running the tipc benchmark
program. It looks like a lock is not dropped sometimes in
nametbl_withdraw.  Also we ran into a situation where a port is dropped
from the congested list but is still referenced in the chain. This was
causing us to panic if the referenced port was deleted, and then the
referencing port was deleted while still congested.

cvs diff -u name_table.c
Index: name_table.c
===================================================================
RCS file: /cvsroot/tipc/source/unstable/net/tipc/name_table.c,v
retrieving revision 1.13
diff -u -r1.13 name_table.c
--- name_table.c	27 Apr 2004 22:44:43 -0000	1.13
+++ name_table.c	28 Apr 2004 17:50:36 -0000
@@ -907,6 +907,7 @@
 		kfree(publ);
 		return 1;
 	}
+	write_unlock_bh(&nametbl_lock); 
 	return 0;
 }
 
cvs diff -u link.c
Index: link.c
===================================================================
RCS file: /cvsroot/tipc/source/unstable/net/tipc/link.c,v
retrieving revision 1.18
diff -u -r1.18 link.c
--- link.c	27 Apr 2004 22:35:25 -0000	1.18
+++ link.c	28 Apr 2004 17:51:02 -0000
@@ -474,6 +474,14 @@
 		port = next;
 	}
 	this->first_waiting_port = port;
+
+	/*
+	 * Make sure that this port isn't pointing at
+	 * any port just removed from congestion
+	 */
+	if (port) {
+		port->prev_waiting = 0;
+	}
  exit:
         spin_unlock_bh(&port_lock);
 }

-- 
Mark Haverkamp <ma...@os...>

Re: [Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

/jon

Mark Haverkamp wrote:


On Tue, 2004-04-27 at 16:58, Jon Maloy wrote:

  

Hi all,

I just uploaded a new file release to SourceForge.

    



  

Has anybody tested the management interface all the way (Mark?)

    



I've been trying it out.  I'm still having trouble in some

circumstances.  Sometimes, while the tipc benchmark program is running,

and I try to get information from the management interface I'll get

stuck waiting in poll.  If I kill the server side while stuck, the

client side will freeze and requires a reboot.  If I kill the program

waiting on the management interface first and then kill the server side

program, I have no problems. I haven't gotten too far into figuring out

what is going on with this one yet but it sounds like a lock problem. 

Especially since it only seems to happen with SMP.

This may be one of the problems I solved in tipc-1.3.10: If you kill a
node while
there are pending connections to it, the auto-disconnect at the other
end will hit 
the node-lock a second time in the same thread, in
"nodesub_unsubscribe()".
I fixed this by making it unecessary to unlink the node-subscriber
objects
within the "handle_node_down()" upcall, -all such subscriptions are
one-shot
anyway. 
The solution is not totally satisfactory, because the "abort_self" call
and
consequenctly the dispatcher upcall is called with the node lock set.
I found no better solution right now, and it works.

Do you still have this problem with tipc-1.3.10 ?








  

Has anybody tried the SOCK_STREAM interface yet ?

What is the status of the multicast code ? (there were some discussions

about who should receive the messages last week, -has this been followed
up

with corrections) ?

The reliable broadcast code ?

    



  

Are there any known bugs/problems that have not been corrected yet.

    



I have enclosed a couple small patches that were found while debugging

access to the management interface while running the tipc benchmark

program. It looks like a lock is not dropped sometimes in

nametbl_withdraw.  Also we ran into a situation where a port is dropped

from the congested list but is still referenced in the chain. This was

causing us to panic if the referenced port was deleted, and then the

referencing port was deleted while still congested.

Good. I am pretty sure I have seen that problem, but only once. Let's
hope 
it is solved now.






cvs diff -u name_table.c

Index: name_table.c

===================================================================

RCS file: /cvsroot/tipc/source/unstable/net/tipc/name_table.c,v

retrieving revision 1.13

diff -u -r1.13 name_table.c

--- name_table.c	27 Apr 2004 22:44:43 -0000	1.13

+++ name_table.c	28 Apr 2004 17:50:36 -0000

@@ -907,6 +907,7 @@

 		kfree(publ);

 		return 1;

 	}

+	write_unlock_bh(&nametbl_lock); 

 	return 0;

 }

 

cvs diff -u link.c

Index: link.c

===================================================================

RCS file: /cvsroot/tipc/source/unstable/net/tipc/link.c,v

retrieving revision 1.18

diff -u -r1.18 link.c

--- link.c	27 Apr 2004 22:35:25 -0000	1.18

+++ link.c	28 Apr 2004 17:51:02 -0000

@@ -474,6 +474,14 @@

 		port = next;

 	}

 	this->first_waiting_port = port;

+

+	/*

+	 * Make sure that this port isn't pointing at

+	 * any port just removed from congestion

+	 */

+	if (port) {

+		port->prev_waiting = 0;

+	}

  exit:

         spin_unlock_bh(&port_lock);

 }



  

Re: [Tipc-discussion] New file release tipc-1.3.10

[Mark H. <ma...@os...>]

On Wed, 2004-04-28 at 13:01, Jon Maloy wrote:
> /jon
> 
> Mark Haverkamp wrote:
> > On Tue, 2004-04-27 at 16:58, Jon Maloy wrote:
> >   
> > > Hi all,
> > > I just uploaded a new file release to SourceForge.
> > >     
> >   
> > > Has anybody tested the management interface all the way (Mark?)
> > >     
> > I've been trying it out.  I'm still having trouble in some
> > circumstances.  Sometimes, while the tipc benchmark program is running,
> > and I try to get information from the management interface I'll get
> > stuck waiting in poll.  If I kill the server side while stuck, the
> > client side will freeze and requires a reboot.  If I kill the program
> > waiting on the management interface first and then kill the server side
> > program, I have no problems. I haven't gotten too far into figuring out
> > what is going on with this one yet but it sounds like a lock problem. 
> > Especially since it only seems to happen with SMP.
> This may be one of the problems I solved in tipc-1.3.10: If you kill a
> node while
> there are pending connections to it, the auto-disconnect at the other
> end will hit 
> the node-lock a second time in the same thread, in
> "nodesub_unsubscribe()".
> I fixed this by making it unecessary to unlink the node-subscriber
> objects
> within the "handle_node_down()" upcall, -all such subscriptions are
> one-shot
> anyway. 
> The solution is not totally satisfactory, because the "abort_self"
> call and
> consequenctly the dispatcher upcall is called with the node lock set.
> I found no better solution right now, and it works.
> 
> Do you still have this problem with tipc-1.3.10 ?

I am seeing this with the latest cvs code.

Mark.
-- 
Mark Haverkamp <ma...@os...>

Re: [Tipc-discussion] New file release tipc-1.3.10

[Mark H. <ma...@os...>]

On Tue, 2004-04-27 at 16:58, Jon Maloy wrote:
> Hi all,
> I just uploaded a new file release to SourceForge.
> I have corrected several bug related to message bundling and
> link changeover, plus a very serious lock bug that sometimes
> caused my processors to hang when they lost contact with each
> other.
> I also corrected a bug in the name/network subscription code,
> so you will now receive an event for all overlapping, already
> existing publications, not only the first one found ;-).
> Some left-out symbols were finally exported as they should.

[ ... ]

> Also, I plan to announce TIPC at the LKML by the end of this week. We must
> admit that the code is still in beta status, but as I understand it this 
> should
> be no problem.

There are still a few things that I think will have to be addressed from
a style/structure point of view before requesting that tipc be
considered for inclusion in the main kernel tree.

Some files still have non-standard indentation (name_table.c for
instance).  There is a shell script in the kernel tree called Lindent
that runs indent to format source files.

The style documents that I have seen say to use the list_head macros
everywhere and not use other methods of linked lists.

There shouldn't be ifdefs in .c code

You may have seen documents like these already, but I found a few
style/how-to documents on the web for linux kernel/driver code that may
be helpful:

http://graphics.cs.ucdavis.edu/~brettw/insight/linuxcodingstyle.html
http://www.linuxjournal.com/article.php?sid=5780
http://www.linuxdevices.com/articles/AT5340618290.html
http://people.redhat.com/arjanv/olspaper.pdf

Also, I think that in the long run, that the 2.4/2.6 ifdef code will
have to be removed and the macro wrappers for things like
kmalloc(k_malloc) will have to be removed.

When you send an announcement, You should also send to
ne...@os... where linux network code is discussed.


-- 
Mark Haverkamp <ma...@os...>

Re: [Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

Do you suggest that we delay the LKML-announcement until we have done
these changes ? To replace all the linked lists is intrusive, and it may

take weeks re-stabilize the code. Otherwise, see my comments below.

Regards /jon

Mark Haverkamp wrote:


On Tue, 2004-04-27 at 16:58, Jon Maloy wrote:

  

Hi all,

I just uploaded a new file release to SourceForge.

I have corrected several bug related to message bundling and

link changeover, plus a very serious lock bug that sometimes

caused my processors to hang when they lost contact with each

other.

I also corrected a bug in the name/network subscription code,

so you will now receive an event for all overlapping, already

existing publications, not only the first one found ;-).

Some left-out symbols were finally exported as they should.

    



[ ... ]



  

Also, I plan to announce TIPC at the LKML by the end of this week. We
must

admit that the code is still in beta status, but as I understand it this


should

be no problem.

    



There are still a few things that I think will have to be addressed from

a style/structure point of view before requesting that tipc be

considered for inclusion in the main kernel tree.



Some files still have non-standard indentation (name_table.c for

instance).  There is a shell script in the kernel tree called Lindent

that runs indent to format source files.

I ran Lindent over all the code a couple of months ago, but some code
has been
added since then. This should be an easy task.






The style documents that I have seen say to use the list_head macros

everywhere and not use other methods of linked lists.

I  generally use linear linked lists, with a zero "prev" pointer for the
first element and a zero "next" pointer for the last, while the the
macros
support only circular lists, as far as I have seen. I have accepted that
we
may use this list support, but have been reluctant to change it in
already
working code, because of the hazard such a redesign involves. In some
cases, like the link's send queue, I think this is impractical and will
cost
performance. 
Let's put this on our TODO list, but fuurther stabilization must have
first 
priority now.






There shouldn't be ifdefs in .c code

The ones that are there now are for excluding code that should be
configurable
from the Kconfig file. Is there any other way ?






You may have seen documents like these already, but I found a few

style/how-to documents on the web for linux kernel/driver code that may

be helpful:



http://graphics.cs.ucdavis.edu/~brettw/insight/linuxcodingstyle.html
<http://graphics.cs.ucdavis.edu/~brettw/insight/linuxcodingstyle.html> 

http://www.linuxjournal.com/article.php?sid=5780
<http://www.linuxjournal.com/article.php?sid=5780> 

http://www.linuxdevices.com/articles/AT5340618290.html
<http://www.linuxdevices.com/articles/AT5340618290.html> 

http://people.redhat.com/arjanv/olspaper.pdf
<http://people.redhat.com/arjanv/olspaper.pdf> 



Also, I think that in the long run, that the 2.4/2.6 ifdef code will

I see there is one I forgot to remove in driver.c, otherwise I think
this is already
done.




have to be removed and the macro wrappers for things like

kmalloc(k_malloc) will have to be removed.



When you send an announcement, You should also send to

ne...@os... <mailto:ne...@os...>  where linux network code
is discussed.





  

Re: [Tipc-discussion] New file release tipc-1.3.10

[Mark H. <ma...@os...>]

On Thu, 2004-04-29 at 08:15, Jon Maloy wrote:
> Do you suggest that we delay the LKML-announcement until we have done
> these changes ? To replace all the linked lists is intrusive, and it
> may 
> take weeks re-stabilize the code. Otherwise, see my comments below.
> 
> Regards /jon
> 

[ ... ]

> > There shouldn't be ifdefs in .c code
> The ones that are there now are for excluding code that should be
> configurable
> from the Kconfig file. Is there any other way ?

I think that this is supposed to be done in header files. Or in some
cases they aren't needed.  I don't think that the ifdef MODULE code is
needed, for instance, since module.h contains the ifdef inside to do the
right thing.

> > You may have seen documents like these already, but I found a few
> > style/how-to documents on the web for linux kernel/driver code that may
> > be helpful:
> > 
> > http://graphics.cs.ucdavis.edu/~brettw/insight/linuxcodingstyle.html
> > http://www.linuxjournal.com/article.php?sid=5780
> > http://www.linuxdevices.com/articles/AT5340618290.html
> > http://people.redhat.com/arjanv/olspaper.pdf
> > 
> > Also, I think that in the long run, that the 2.4/2.6 ifdef code will
> I see there is one I forgot to remove in driver.c, otherwise I think
> this is already
> done.

You are right.  I didn't count how many there were.


-- 
Mark Haverkamp <ma...@os...>

Re: [Tipc-discussion] New file release tipc-1.3.10

[Daniel M. <da...@os...>]

On Thu, 2004-04-29 at 08:15, Jon Maloy wrote:
> Do you suggest that we delay the LKML-announcement until we have done
> these changes ? To replace all the linked lists is intrusive, and it
> may 
> take weeks re-stabilize the code. Otherwise, see my comments below.
> 
> Regards /jon
> 

It is difficult to say what the reaction will be, but I suggest
you look over the links Mark sent and see what you think.  It
is your code after all.  Using the common list code does make
it easy to follow the code and we did find a bug recently.  Using
the standard list macros would have prevented that bug.

Daniel

Re: [Tipc-discussion] New file release tipc-1.3.10

[Mark H. <ma...@os...>]

On Thu, 2004-04-29 at 08:15, Jon Maloy wrote:
> Do you suggest that we delay the LKML-announcement until we have done
> these changes ? To replace all the linked lists is intrusive, and it
> may 
> take weeks re-stabilize the code. Otherwise, see my comments below.
> 
> Regards /jon

Steve Hemminger said to send to  lin...@vg... also.

> 

[...]


> > Also, I think that in the long run, that the 2.4/2.6 ifdef code will
> I see there is one I forgot to remove in driver.c, otherwise I think
> this is already
> done.

I talked with a couple people here about the 2.4/2.6 code directories. 
They said that code submitted for review should be for 2.6 only and not
have it set up for both versions of the kernel.

> >   
-- 
Mark Haverkamp <ma...@os...>

Re: [Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

Ok, I think we should do the following:

- Remove the 2.4/2.6 directory, and put everything back in one
directory.
   I talked to some people from MontaVista a few days ago, and it seems
like 
   the 2.4 support is not that important for them, so we can skip that
altogether.
- Remove all "placeholder"  code, like the whole contents of proc.c, and
the 
   inter-cluster configuration code in cfg.c. This code  can be
re-introduced once 
   it is redesigned and functional. We get rid of almost all the #ifdefs
this way. 
  The MODULE flag can also easily be removed, as Mark pointed out. If
there is
   more we should consider each case.
- We go systematically through the code and replace (where possible) the
linear 
   linked lists with the Linux supported ditto. 
   The exceptions are places in the core where we  have queues (there
are two such, 
   I think) of  sk_buffs. To retain a minimum  level of OS portability
for the core 
   code I still insist that sk_buffs should only be accessed via
macros/inlines, inclusive 
   any next/prev pointers.
- Re-run Lindent on everything. 

Anything more ?

I will delay the announcment at LKML until this is done. (-Anything to
avoid being 
welcomed by flames from the community ;-) )

Can I hope to get any help with this ?

Regards /Jon
   

Mark Haverkamp wrote:


On Thu, 2004-04-29 at 08:15, Jon Maloy wrote:

  

Do you suggest that we delay the LKML-announcement until we have done

these changes ? To replace all the linked lists is intrusive, and it

may 

take weeks re-stabilize the code. Otherwise, see my comments below.



Regards /jon

    



Steve Hemminger said to send to   lin...@vg...
<mailto:lin...@vg...>  also.



  


[...]





  

Also, I think that in the long run, that the 2.4/2.6 ifdef code will

      

I see there is one I forgot to remove in driver.c, otherwise I think

this is already

done.

    



I talked with a couple people here about the 2.4/2.6 code directories. 

They said that code submitted for review should be for 2.6 only and not

have it set up for both versions of the kernel.



  

  

      

Re: [Tipc-discussion] New file release tipc-1.3.10

[Mark H. <ma...@os...>]

On Thu, 2004-04-29 at 10:28, Jon Maloy wrote:
> Ok, I think we should do the following:
> 
> - Remove the 2.4/2.6 directory, and put everything back in one
> directory.
>    I talked to some people from MontaVista a few days ago, and it
> seems like 
>    the 2.4 support is not that important for them, so we can skip that
> altogether.
> - Remove all "placeholder"  code, like the whole contents of proc.c,
> and the 
>    inter-cluster configuration code in cfg.c. This code  can be
> re-introduced once 
>    it is redesigned and functional. We get rid of almost all the
> #ifdefs this way. 
>   The MODULE flag can also easily be removed, as Mark pointed out. If
> there is
>    more we should consider each case.
> - We go systematically through the code and replace (where possible)
> the linear 
>    linked lists with the Linux supported ditto. 
>    The exceptions are places in the core where we  have queues (there
> are two such, 
>    I think) of  sk_buffs. To retain a minimum  level of OS portability
> for the core 
>    code I still insist that sk_buffs should only be accessed via
> macros/inlines, inclusive 
>    any next/prev pointers.
> - Re-run Lindent on everything. 
> 
> Anything more ?
> 
> I will delay the announcment at LKML until this is done. (-Anything to
> avoid being 
> welcomed by flames from the community ;-) )
> 
> Can I hope to get any help with this ?

I can help out with the linked list conversion.

Mark.

> 
> Regards /Jon

-- 
Mark Haverkamp <ma...@os...>

Re: [Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

Thanks. I will take care of the rest, and continue trouble-shooting
on parallel-link communication, which is still a little shaky.

/jon

Mark Haverkamp wrote:


On Thu, 2004-04-29 at 10:28, Jon Maloy wrote:

  

Ok, I think we should do the following:



- Remove the 2.4/2.6 directory, and put everything back in one

directory.

   I talked to some people from MontaVista a few days ago, and it

seems like 

   the 2.4 support is not that important for them, so we can skip that

altogether.

- Remove all "placeholder"  code, like the whole contents of proc.c,

and the 

   inter-cluster configuration code in cfg.c. This code  can be

re-introduced once 

   it is redesigned and functional. We get rid of almost all the

#ifdefs this way. 

  The MODULE flag can also easily be removed, as Mark pointed out. If

there is

   more we should consider each case.

- We go systematically through the code and replace (where possible)

the linear 

   linked lists with the Linux supported ditto. 

   The exceptions are places in the core where we  have queues (there

are two such, 

   I think) of  sk_buffs. To retain a minimum  level of OS portability

for the core 

   code I still insist that sk_buffs should only be accessed via

macros/inlines, inclusive 

   any next/prev pointers.

- Re-run Lindent on everything. 



Anything more ?



I will delay the announcment at LKML until this is done. (-Anything to

avoid being 

welcomed by flames from the community ;-) )



Can I hope to get any help with this ?

    



I can help out with the linked list conversion.



Mark.



  

Regards /Jon

    



  

Re: [Tipc-discussion] New file release tipc-1.3.10

[Stephen H. <she...@os...>]

Overall TIPC 1.3 comments:

* README seems out of date

* Configuration by module parameters is bad because in most
  standard environments, module loading is done automatically.

* Need 2.6 only, no standalone build environment
  No extra CFLAGS in Makefile please

* Hard coding clusters, zones, nodes maximum makes it impossible
  for distro vendors to ship a standard kernel.

* having own caching allocator, if you insist on caching
  then use kmem_cache_alloc.

* it links to network devices, but does not handle up/down/unregister
  notifications.  THIS IS A SHOWSTOPPER

* holds references to network devices without holding reference counts.
  THIS IS A SHOWSTOPPER

* why own version of wait_event_interruptible?

* excessive use of /proc to provide interface.  /proc is okay
  as a statistical interface, not a control channel!

* your socket locking is a mess. What is wrong with lock_sock()?

* you invent your own socket queues, there are standard macros
  to do that.

* get rid of all the macros in tipc_adapt.h
	err, warn, info, dbg, ...
  use the standard macros in kernel.h

* hardcoding device names 'eth0,eth1,eth2,...' in code and
  configuration is unacceptable.  What about other media types
  and netdevice names can be anything the administrator wants.

tipc.h
- cplusplus support not usually done in linux header files

- get rid of typedef's for tipc_net_addr_t, tipc_ref_t
  kernel standard is to not use typedef's if possible

- tipc_addr,zone,cluster,node
	using inline instead would gain type checking

- no IPV6 support, would probably be a show stopper for many people

addr.c
* global variable, buffers's waiting to be overwritten, ugh.


* k_time_ms == jiffies_to_clock_t

* The whole TIPC_MAGIC in header crap looks a trap waiting
  to happen

That is enough for now, that is just what I found in a 15min
quick look.  If Viro saw it, he wouldn't be as nice ;-)

Re: [Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

Thank you for the comments. Most of the problems you point out are
omissions that are relatively easy to fix, as far as I can see, but some
require more explanation, see below. Some constructive suggestions
would also be appreciated, especially in the cases where you seem
to have particularly dislike for the code.

/jon


Stephen Hemminger wrote:

>Overall TIPC 1.3 comments:
>
>* README seems out of date
>
>* Configuration by module parameters is bad because in most
>  standard environments, module loading is done automatically.
>
What do you suggest ? With manual insmod loading this very practical.

>
>* Need 2.6 only, no standalone build environment
>  No extra CFLAGS in Makefile please
>
It was never the intention that the current directory structure and make
support at SF would be the one eventually integrated into the kernel. But
we do need a standalone build support, so I guess we need to keep the
code in two different structures in the future.

>
>* Hard coding clusters, zones, nodes maximum makes it impossible
>  for distro vendors to ship a standard kernel.
>
>* having own caching allocator, if you insist on caching
>  then use kmem_cache_alloc.
>
The only caching we do is for ready-built sk_buffs. I don't see how this 
can
be achieved with kmem_cache_alloc, but maybe I am missing something ?

>
>* it links to network devices, but does not handle up/down/unregister
>  notifications.  THIS IS A SHOWSTOPPER
>
>* holds references to network devices without holding reference counts.
>  THIS IS A SHOWSTOPPER
>
No problem. Something I have missed.

>
>* why own version of wait_event_interruptible?
>
I guess you mean wait_event_interruptible_timeout. This did not exist in
the versions of 2.4 that we support, so it is just legacy. This has been
fixed in all files except proc.c, which is anyway to be removed, -see
previous mail.

>
>* excessive use of /proc to provide interface.  /proc is okay
>  as a statistical interface, not a control channel!
>
See above.

>
>* your socket locking is a mess. What is wrong with lock_sock()?
>  
>
>* you invent your own socket queues, there are standard macros
>  to do that.
>
>* get rid of all the macros in tipc_adapt.h
>	err, warn, info, dbg, ...
>  use the standard macros in kernel.h
>
The Linux standard macros are not sufficent. Neither is Ethereal.
This is the most important tool I have for trouble shooting.
Try to trace a lost packet when the loss is detected one minute
and 2 million packets later, and you will understand what I mean.

>
>* hardcoding device names 'eth0,eth1,eth2,...' in code and
>  configuration is unacceptable.  What about other media types
>  and netdevice names can be anything the administrator wants.
>
Yes, I have been aware of that, but strings are no good either. Any
suggestions?

>
>tipc.h
>- cplusplus support not usually done in linux header files
>
>- get rid of typedef's for tipc_net_addr_t, tipc_ref_t
>  kernel standard is to not use typedef's if possible
>
>- tipc_addr,zone,cluster,node
>	using inline instead would gain type checking
>
>- no IPV6 support, would probably be a show stopper for many people
>
It is in the pipe.

>
>addr.c
>* global variable, buffers's waiting to be overwritten, ugh.
>
>
>* k_time_ms == jiffies_to_clock_t
>
>* The whole TIPC_MAGIC in header crap looks a trap waiting
>  to happen
>
I guess you refer to the mangement interface header. We need some
minimum level of security, apart from disabling the whole functionality,
which was meant to be configurable, and this was the simple solution
I came up with.  If you literally mean this may cause a trap, I don't
understand what you mean. A wrong "magic" will cause a command
rejection, and that is it.

Please elaborate "crap".  I realize that the management interface
is not perfect, but the fact that you don't understand it in five
seconds does not either imply that it is all wrong.

>
>That is enough for now, that is just what I found in a 15min
>quick look.  If Viro saw it, he wouldn't be as nice ;-)
>  
>

Re: [Tipc-discussion] New file release tipc-1.3.10

[Daniel M. <da...@os...>]

On Thu, 2004-04-29 at 08:15, Jon Maloy wrote:
> Do you suggest that we delay the LKML-announcement until we have done
> these changes ? To replace all the linked lists is intrusive, and it
> may 
> take weeks re-stabilize the code. Otherwise, see my comments below.
> 
> Regards /jon
> 

Jon,

One other thing about TIPC that Mark and I have noticed that might
cause mainline acceptance problems is the:

	ref_lock_acquire(void *object,spinlock_t ** lock)
	ref_lock_deref()

	namesub_lock_deref()
	ref_unlock_discard(s->publ.s.ref);


	port_lock_deref()
	spin_unlock_bh(this->publ.lock);
	ref_unlock_discard(ref);



2 big concerns:

1. This hides the locking since a pointer is set to the "real" lock.
   The usage above just does not seem clear on what is being locked.
  
2. The lock is locked in one function and returned with the spinlock
   held and then unlock() in a different function.

Maybe more comments on functions saying which locks are assumed held
and which locks are unlocked that came in locked might clear up some
of the confusion.

Thanks,

Daniel

Re: [Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

I realize the problem, but I have difficulties with seeing 
how we can do otherwise if we want lock granularity 
per node instance. 
I tried to make it clear via the function names, but it can certainly 
be improved by better comments on this. (I already have such 
comments at some places.)  
I think improved comments is the best we can do, unless somebody 
has a  better idea.

Regards /Jon

Daniel McNeil wrote:


On Thu, 2004-04-29 at 08:15, Jon Maloy wrote:

  

Do you suggest that we delay the LKML-announcement until we have done

these changes ? To replace all the linked lists is intrusive, and it

may 

take weeks re-stabilize the code. Otherwise, see my comments below.



Regards /jon



    



Jon,



One other thing about TIPC that Mark and I have noticed that might

cause mainline acceptance problems is the:



	ref_lock_acquire(void *object,spinlock_t ** lock)

	ref_lock_deref()



	namesub_lock_deref()

	ref_unlock_discard(s->publ.s.ref);





	port_lock_deref()

	spin_unlock_bh(this->publ.lock);

	ref_unlock_discard(ref);







2 big concerns:



1. This hides the locking since a pointer is set to the "real" lock.

   The usage above just does not seem clear on what is being locked.

  

2. The lock is locked in one function and returned with the spinlock

   held and then unlock() in a different function.



Maybe more comments on functions saying which locks are assumed held

and which locks are unlocked that came in locked might clear up some

of the confusion.



Thanks,



Daniel