Re: MPLS support for DragonFly

["Nuno Antunes" <nuno.antunes@xxxxxxxxx>]

Please find my comments inline, below.

On Wed, Jul 2, 2008 at 9:42 AM, Jeffrey Hsu <hsu@freebsd.org> wrote:
> I like it.  Some comments:
>
> 1. rt_setshim() should be static.

Done.

> 2. Make that rt_setshims(rt, rtinfo->rt_info) to simplify the calling
> interface.

Done.

> 3. Remove all the null checks in rt_setshims(), because it's only called
> from one place and after a bzero().

Done.

> 3. There's an extra newline at the end of sbin/route.c.

Removed.

> 4. Don't need MPLS config file.  Just add to LINT config file.

Removed.

> 5. What's the multiplexing strategy in terms of distributing work
> and avoiding locking by using per-cpu data structures?

The work is distributed by the available processors based on the
incoming label xor'ed to to the incoming interface index. I didn't
give much thought to this, probably there are better ways to fairly
distribute work. The overall mechanism was adapted from the IP
implementation (your implementation actually :), with routing tables
replicated for each cpu and messages propagated to all cpus when there
are routing updates. At least that was my goal, have I missed
something?

> 6. Instead of a separate mpls_output(), can you factor out the MPLS
> output process from the call to the underlying interface, as in
> something like
>
> #ifdef MPLS
>  struct rtentry *send_route = ro->ro_rt;       /* copy-in/copy-out parameter */
>
>  if (!mpls_output_process(ifp, m, &dst, &send_route))
>        goto done;
> #endif
>
>  error = ifp->if_output(ifp, m, (struct sockaddr *)dst, send_route);
>
>  /*
>   * Returns FALSE if no further output processing required.
>   */
>  bool_t
>  mpls_output_process()
>  {
>
>        if (!(send_route->rt_flags & RTF_MPLSOPS))
>                return TRUE;
>
>        do MPLS processing
>        return FALSE if stack empty or on mpls_push() or mpls_swap() error
>
>        return TRUE;
>  }
>

Done.

New patch at [1] or incremental version relative to the last one at [2].

Many thanks for the feedback!
nuno

[1] http://leaf.dragonflybsd.org/~nant/wip/mpls-20080703.patch
[2] http://leaf.dragonflybsd.org/~nant/wip/mpls-20080703-incremental.patch