Date: Sun, 3 May 2009 22:03:06 -0600 From: Will Andrews <will@firepipe.net> To: "Bruce M. Simpson" <bms@freebsd.org> Cc: freebsd-net@freebsd.org Subject: Re: CARP as a module; followup thoughts Message-ID: <2aada3410905032103g734e7025nad7f7b13137572ed@mail.gmail.com> In-Reply-To: <49EF11E8.508@FreeBSD.org> References: <2aada3410904212216o128e1fdfx8c299b3531adc694@mail.gmail.com> <49EF11E8.508@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Apr 22, 2009 at 6:47 AM, Bruce M. Simpson <bms@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. =A0It 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 a= n > 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2aada3410905032103g734e7025nad7f7b13137572ed>