Re: CARP as a module; followup thoughts
Will Andrews wrote: Thank you very much for your feedback. I have implemented your suggestion as follows: http://firepipe.net/patches/carp-as-module-20090503-2.diff Great stuff. Overall this does make things that much cleaner. This version doesn't reinvent the wheel as far as registering the protocol goes. Personally, I think that notwithstanding your other objection, this should get committed, since it is a step in the right direction (perhaps minus the netinet6/in6_proto.c change that adds spacers). One step at a time! Yeah, I wouldn't worry about it too much for now. It is something which would be nice to have -- some NICs will perfect hash in hardware on more than one MAC address -- but I believe we've got other issues in this area to do with per-AF locking, which would probably be touched by exactly the issues you raise in the last part of your post... well spotted... carp_encapcheck() is simplistic, but probably suffices (maybe also check to see if the vh MAC matches). This patch does work fine with my test setup, one system using GENERIC+if_carp and the other using a static build without the patch. I'll have to take your word for that as I'm not using CARP just at the moment. I had to touch the mcast setup for the IPv6 SSM implementation. All compiles OK, but I haven't tested the code other than loading it. Only IPv6 multicast group setup should be affected. Does your patch apply against these revisions OK? I also found a memory leak in the original code, where it calls free(cif, M_IFADDR), which is wrong, it should be free(cif, M_CARP), since the original malloc uses M_CARP -- this fix is also included in the patch above. Great stuff. Can this bug fix be merged separately, i.e. before other code is committed? That way it can get merged back to -STABLE more quickly, once RELENG_7 is unfrozen. ... Another way would be to have a separate list/hash table for virtual lladdr's (ifnet.ifvirt_lladdrhead?). I considered that but it seems better and more general to simply upgrade ifnet.if_addrhead. It would be good to have a more general code path for stuff like this to benefit from using the perfect hash filters in modern NICs, the main thing is that everything continues to work with no regressions :-) Thanks for the effort you've put into this, it will certainly help a lot of folk to be able to ship a CARP-capable GENERIC kernel. cheers, BMS ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
Re: CARP as a module; followup thoughts
On Mon, May 4, 2009 at 10:04 AM, Bruce Simpson b...@incunabulum.net wrote: I'll have to take your word for that as I'm not using CARP just at the moment. I had to touch the mcast setup for the IPv6 SSM implementation. All compiles OK, but I haven't tested the code other than loading it. Only IPv6 multicast group setup should be affected. Does your patch apply against these revisions OK? It should. I am using git to develop these patches. I just did another sync (to r191794) and the diff from svn to my local git branch is the same as the patch I posted last night, so I presume it will apply to a fresh svn checkout of -current as of that revision. Great stuff. Can this bug fix be merged separately, i.e. before other code is committed? That way it can get merged back to -STABLE more quickly, once RELENG_7 is unfrozen. Yes, I can generate a separate patch for that one. If I were able to commit it myself, I'd certainly be doing it the way you suggest. I'd also suggest a more aggressive MFC timing for the free() bug fix than for the module feature (perhaps 3 days vs. 1-2 months, as 7.2R is now out). I am going to backport this patch to RELENG_7. Because of the way it is implemented, I believe it should be safe to MFC. It would be good to have a more general code path for stuff like this to benefit from using the perfect hash filters in modern NICs, the main thing is that everything continues to work with no regressions :-) Thanks for the effort you've put into this, it will certainly help a lot of folk to be able to ship a CARP-capable GENERIC kernel. Indeed, regressions will be difficult to prevent. I'm planning to work on virtual lladdrs for a bit to see if I can find a suitable solution for the problem. If nothing else, I think it provides a reasonable method for getting rid of carp_forus(), and possibly for implementing carpdev. Thanks, --Will. ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
Re: CARP as a module; followup thoughts
On Wed, Apr 22, 2009 at 6:47 AM, Bruce M. Simpson b...@freebsd.org wrote: Hi, Will Andrews wrote: Hello, I've written a patch (against 8.0-CURRENT as of r191369) which makes it possible to build, load, run, unload CARP as a module, using the GENERIC kernel. It can be obtained from: http://firepipe.net/patches/carp-as-module-20090421.diff There's no need to implement the in*_proto_register() stuff in that patch, you should just be able to re-use the encap_attach_func() functions. Look at how PIM is implemented in ip_mroute.c for an example. Other than that it looks like a good start... but would hold off on committing as-is. the more general case of registering a MAC address on an interface should be considered. Thank you very much for your feedback. I have implemented your suggestion as follows: http://firepipe.net/patches/carp-as-module-20090503-2.diff This version doesn't reinvent the wheel as far as registering the protocol goes. Personally, I think that notwithstanding your other objection, this should get committed, since it is a step in the right direction (perhaps minus the netinet6/in6_proto.c change that adds spacers). One step at a time! carp_encapcheck() is simplistic, but probably suffices (maybe also check to see if the vh MAC matches). This patch does work fine with my test setup, one system using GENERIC+if_carp and the other using a static build without the patch. I also found a memory leak in the original code, where it calls free(cif, M_IFADDR), which is wrong, it should be free(cif, M_CARP), since the original malloc uses M_CARP -- this fix is also included in the patch above. I've looked at the general case of registering a MAC address. I was going to try to include that change in this patch, but after reading the interface code for a while, I realized there isn't a general way to do that that seems settled. So it appears there needs to be a discussion on how to accomplish this. So, in struct ifnet, there is a field called if_addrhead which is a list of struct ifaddr's. This appears to be used for the general case of all addresses registered to a particular interface (AF_LINK aka lladdr's, plus AF_INET, AF_INET6, etc.). Now, we could use this with TAILQ_INSERT()'s for each virtual AF_LINK, and replace the applicable checks for IF_LLADDR(ifp) with a function that searches ifnet.if_addrhead for all AF_LINK entries and comparing the addresses to determine a match. Problem is, that's more O(n) than O(1), which is probably not acceptable. Perhaps a better way would be to replace ifnet.if_addrhead with a hash table for O(log n) search complexity, and possibly having separate hash tables for AF_LINK vs. other address families? Or maybe even one for each address family. That's probably overkill. There does seem to be a need to distinguish physical AF_LINKs from virtual though, since each physical interface driver uses IF_LLADDR(ifp) to refer to its physical address. Possibly ifaddr.ifa_flags could be used to make this distinction (though it seems to be used mainly for routing flags), but probably leave ifnet.if_addr as is for that purpose. Another way would be to have a separate list/hash table for virtual lladdr's (ifnet.ifvirt_lladdrhead?). I considered that but it seems better and more general to simply upgrade ifnet.if_addrhead. Regards, --Will. ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
Re: CARP as a module; followup thoughts
On 2009.04.21 23:16:58 -0600, Will Andrews wrote: Hello, I've written a patch (against 8.0-CURRENT as of r191369) which makes it possible to build, load, run, unload CARP as a module, using the GENERIC kernel. It can be obtained from: http://firepipe.net/patches/carp-as-module-20090421.diff I don't have any comments on the specific patch, but with my FreeBSD end-user hat, being able to have CARP in GENERIC would be really great. This would allow me to update my systems which use CARP with freebsd-update without manually compiling a kernel. So if the patch doesn't penalize the non-CARP case much, I think it would be great to have this functionality for now, even if it's not the way to go in the long run. -- Simon L. Nielsen ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
Re: CARP as a module; followup thoughts
Will Andrews wrote: I'm sure most of this sounds like rambling from a crazed lunatic or something, but I'm also sure most who understand my patch agree that it isn't the nicest of ways to make it possible to load carp (or any other protocol) as a module. Not at all. It is a mess to be sure. One of the criticisms of Netgraph is that it is poorly understood outside of its immediate developer community. The BSD networking stack has a number of textbooks written for it, Netgraph does not, and it probably factored into the decision of Itronix to sponsor a from-scratch implementation of Bluetooth for NetBSD -- netgraph has been considered 'a bridge too far', to score a cheesy pun. It has also been criticised for performance, although I am not in a position to judge either way at the moment, I simply don't have all the information to hand, and am busy doing other things often. I don't have time to look at your patch right now, unfortunately, but can try to make time when less pressed. When I last looked at the CARP hooks, during the ether_input() cleanup, all that was really missing was the ability to register soft MAC addresses in the perfect hash filter entries other than the one programmed into the card (or configured via ifconfig(8) mechanisms). cheers BMS ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
Re: CARP as a module; followup thoughts
Hi, Will Andrews wrote: Hello, I've written a patch (against 8.0-CURRENT as of r191369) which makes it possible to build, load, run, unload CARP as a module, using the GENERIC kernel. It can be obtained from: http://firepipe.net/patches/carp-as-module-20090421.diff There's no need to implement the in*_proto_register() stuff in that patch, you should just be able to re-use the encap_attach_func() functions. Look at how PIM is implemented in ip_mroute.c for an example. Other than that it looks like a good start... but would hold off on committing as-is. the more general case of registering a MAC address on an interface should be considered. cheers, BMS ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
CARP as a module; followup thoughts
Hello,
I've written a patch (against 8.0-CURRENT as of r191369) which makes
it possible to build, load, run, unload CARP as a module, using the
GENERIC kernel. It can be obtained from:
http://firepipe.net/patches/carp-as-module-20090421.diff
Having written this patch, I have some thoughts. First of all, this
patch follows the same pattern of function pointers used by if_lagg,
if_vlan, ng_ether, bpf, and if_bridge. While it works, this approach
(along with that used by the other interfaces) is a hackish way to
implement the interfaces as kernel modules.
It appears that each one needs to have its hooks inserted at a
specific point in the packet processing. So it seems to me that a
better way to do this would be to implement a generic network protocol
interface and have everything that processes packets register its
hooks with that interface. Which if_* seems to do to an extent, but
not enough to meet the requirements of the above-mentioned network
protocols.
More to the point, netinet/in_proto.c netinet6/in6_proto.c use
hardcoded protocol definition structures. Until this diff introduced
in{6,}_proto_{un,}register(), there was no way to define hooks for any
other protocols without hacking these files or compiling with
different options (like DEV_CARP).
I envision a struct ifnet_hooks array that includes hooks that can be
registered by any protocol for packet processing at any point,
including: pre-input, input, post-input, pre-output, output,
post-output, link state change, route, etc. Then each struct ifnet
would contain a list of these pointers, to be configured in a given
order depending on the administrator's needs. The interface would run
through the list for a given stage and run the protocol specific
function pointer to perform its processing at that stage.
Of course, that is probably a much too simplistic idea (there are a
lot of special cases it seems). And the reality is, there is already
something in FreeBSD that makes arbitrary packet processing hook order
possible - netgraph. So why is it FreeBSD allows these modules when
there are netgraph equivalents for all of them (currently, except
CARP)? More to the point, why isn't netgraph used for most (if not
all) packet processing?
Has anyone tried to build a kernel without INET? It's not pretty, and
demonstrates the biases the stack has towards IPv4 vs. other protocols
like IPv6. In other words, there's lots of code that looks like this:
some IPv4 specific stuff
#ifdef INET6
some IPv6 specific stuff
#endif
It would be nice if the stack didn't assume any particular protocol
base; making all protocols optional (except as explicitly defined by
direct dependency) seems a worthy goal.
I think it also might be useful to third party developers if they
didn't have to modify anything in the base kernel to insert a new
protocol in the stack.
I'm sure most of this sounds like rambling from a crazed lunatic or
something, but I'm also sure most who understand my patch agree that
it isn't the nicest of ways to make it possible to load carp (or any
other protocol) as a module.
Regards,
--Will.
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
