CARP as a module; followup thoughts

[Will Andrews]

On Wed, Apr 22, 2009 at 6:47 AM, Bruce M. Simpson <bms at 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.