From owner-freebsd-net@FreeBSD.ORG Mon May 4 04:03:07 2009 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 65AF81065677; Mon, 4 May 2009 04:03:07 +0000 (UTC) (envelope-from will@firepipe.net) Received: from mail-gx0-f162.google.com (mail-gx0-f162.google.com [209.85.217.162]) by mx1.freebsd.org (Postfix) with ESMTP id 17D3D8FC1A; Mon, 4 May 2009 04:03:06 +0000 (UTC) (envelope-from will@firepipe.net) Received: by gxk6 with SMTP id 6so629373gxk.19 for ; Sun, 03 May 2009 21:03:06 -0700 (PDT) MIME-Version: 1.0 Received: by 10.151.122.7 with SMTP id z7mr11190162ybm.140.1241409786438; Sun, 03 May 2009 21:03:06 -0700 (PDT) In-Reply-To: <49EF11E8.508@FreeBSD.org> References: <2aada3410904212216o128e1fdfx8c299b3531adc694@mail.gmail.com> <49EF11E8.508@FreeBSD.org> Date: Sun, 3 May 2009 22:03:06 -0600 Message-ID: <2aada3410905032103g734e7025nad7f7b13137572ed@mail.gmail.com> From: Will Andrews To: "Bruce M. Simpson" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-net@freebsd.org Subject: Re: CARP as a module; followup thoughts X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 May 2009 04:03:07 -0000 On Wed, Apr 22, 2009 at 6:47 AM, Bruce M. Simpson 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.