Date: Wed, 20 Oct 2004 19:25:57 +0200 From: Andre Oppermann <andre@freebsd.org> To: Max Laier <max@love2party.net> Cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sys/sys protosw.h src/sys/kern uipc_domain.cuipc_socket2.c Message-ID: <41769FA5.413832D@freebsd.org> References: <200410191513.i9JFDUbf072176@repoman.freebsd.org> <200410200035.19752.max@love2party.net> <41759D75.1B6BDDC2@freebsd.org> <200410200129.54320.max@love2party.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Max Laier wrote: > > On Wednesday 20 October 2004 01:04, Andre Oppermann wrote: > > Please point them out. We can discuss specifics then instead of creating > > a clout of FUD. > > Okay, that's the last straw. This is not FUD. We are really concerned that you > are introduceing something that is not fully though about. Not exactly. Dynamically registering and unregistering is not the problem. It works without causing panics or races (except when two protcols want to register exactly at the same time which is perfectly solved with GIANT). > We will have problems with this and I really think that it should be backed > out now and fixed before reconsidered! What can be problematic is not the unregistering of an protocol but the subsequent unloading of the module that implemented said protocol. This is where where all this discussion comes from (?). In the case of IPDIVERT I made it unloadable and as you correctly say there can be certain race conditions. The solution is not to back out what I committed but to make IPDIVERT to refuse to unload, *or* to fix IPDIVERT in a way to make it 100% race safe to unload. > Unloading (of divert) has races that can be triggered. Claiming that they will > not be "most of the time" is not exactly the right approach for serious > development. For the record; The risk for a panic I introduced here with IPDIVERT is several squareorders in magnitude lower than the accept() panic fixed a couple of days ago because here it could only happen if someone was actually trying to unload IPDIVERT. It would never happen in any other circumstances. That being said, see above how to fix. > And for pointing out specifics: ip_encap is going to be a lot of fun. And > that's just the most obvious, I could find. I will not scan the tree for more > places, but the ip_icmp.c example shows that you didn't scan the tree > carefully enough before you committed this work without previous discussion. > It seems that you just assume that everything is fine. It really is not! You are on the wrong boat here. ip_encap is an entirely different animal. ip_encap manages its own private protosw to demux many sub-protocols that have accumulated over time on the original GRE IP protocol type. What ip_encap is doing has no relation to the general ip_proto_[un]register function because it can't and doesn't deal with ip_encap's private protosw. And to rebut that I have carelessly thrown some untested and immature piece of code in there; I have in fact looked and thought through this and studied the code that uses it. It fits nicely into the design and intention of the protosw structure concept. That's why I have done this and not some other way. The ip_icmp.c example is the only one in the netinet code. In other places tests are done to check if the function pointer is != NULL. As Harti said the compiler will do whatever it wants. This makes unloading almost any module unsafe, doesn't it? > I understand that you didn't want to slow things down in a bikeshed, but this > is not ready, sorry. No no, things got mixed up wrt. protcol unregistering (which is fine and has its uses) and unloading a module immediatly after it unregistered its protocol. -- Andre
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?41769FA5.413832D>