DragonFlyBSD

:Hi All,
:
:I have this patch [1] sitting in my tree for several months now (just
:caught up with HEAD last weekend) but maybe it's time to ask others
:opinion before it grows spider webs on it :). It adds MPLS (Multi
:Protocol Label Switching) support to DragonFly and still has some
:loose ends but basically it works. For now, only static mpls routes
:are supported. To make it really usable, an LDP and / or RSVP
:implementation would be needed to accompany it. If reviews are
:positive I would like to put it in and commit it maybe after the
:release in order to allow for wider testing.
:
:[1] http://leaf.dragonflybsd.org/~nant/wip/mpls-20080701.patch
:
:Thanks,
:Nuno

It looks pretty clean.  The only thing I see is that you've stuck
    the routing in the ip_output code.  Isn't it supposed to be 
    multi-protocol?  i.e. more of a MAC-like layer routing and not
    necessarily just for IP ?

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

On Tue, Jul 1, 2008 at 11:57 PM, Matthew Dillon
<dillon@apollo.backplane.com> wrote:

:Hi All,
:
:I have this patch [1] sitting in my tree for several months now (just
:caught up with HEAD last weekend) but maybe it's time to ask others
:opinion before it grows spider webs on it :). It adds MPLS (Multi
:Protocol Label Switching) support to DragonFly and still has some
:loose ends but basically it works. For now, only static mpls routes
:are supported. To make it really usable, an LDP and / or RSVP
:implementation would be needed to accompany it. If reviews are
:positive I would like to put it in and commit it maybe after the
:release in order to allow for wider testing.
:
:[1] http://leaf.dragonflybsd.org/~nant/wip/mpls-20080701.patch
:
:Thanks,
:Nuno

It looks pretty clean. The only thing I see is that you've stuck
the routing in the ip_output code. Isn't it supposed to be
multi-protocol? i.e. more of a MAC-like layer routing and not
necessarily just for IP ?

-Matt
Matthew Dillon
<dillon@backplane.com>

Yes, it serves three purposes:
1) It can switch mpls packets, i.e. receive as mpls, swap the label
and forward out again as an mpls packet, or
2) it can work as an ingress node i.e. receive an ip packet, push a
label to it and forward out as mpls packet, or
3) it can work as egress node i.e. receive an explicit-null labeled
mpls packet, pop the label out and process the packet.

Historically, MPLS was supposed to work with ATM too and make use of
vpi/vci as label containers. But what I've seen so far was mostly MPLS
over Ethernet encapsulating IP or layer2 traffic.

Thanks,
Nuno

The patch looks good. Just nit picking: would you please adjust the
style in mpls_demux.c? Things like

struct lwkt_port *
mpls_mport(struct mbuf **mp) {
}

to

struct lwkt_port *
mpls_mport(struct mbuf **mp) {
}

If you are going to commit it, I will commit a fix on ETHER_INPUT2
path, once you have done.

Best Regards,
sephe

Sure, thanks! :)

Ok should i put it in before or after release?

nuno

I am ok to have it before release; don't know other folks opinion tho.

Best Regards,
sephe

Me too.

Some comments ...

• Whitespace in etc/mtree/BSD.include.dist - this file uses spaces for
  some reason.
• Don't include sys/config/MPLS if committing :).
• I don't like limiting label operations in this way, this should be
  solved better IMHO. And is max 3 operations enough? H-VPLS egress
  router which is in same time egress router for fast-reroute TE
  tunnel? :) This isn't objection to commit the patch though ... just food
  of thought.

Wow, so many feedback from everyone, great! I'll have to address this
after work later today. But with regards to the kernel config file,
that got in the patch inadvertently, i don't intend to commit it.

Thanks
nuno

I like it. Some comments:

1. rt_setshim() should be static.

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

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

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

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

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

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;
  }

:
:Sepherosa Ziehau wrote:
:> I am ok to have it before release; don't know other folks opinion tho.
:
:Me too.
:
:--
:Hasso Tepper

Yup.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Please find my comments inline, below.

Done.

Done.

Done.

Removed.

Removed.

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?

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

On Wed, Jul 2, 2008 at 10:15 AM, Hasso Tepper
<bugs@lists.dragonflybsd.org> wrote:

Hasso Tepper <hasso@estpak.ee> added the comment:

Some comments ...

• Whitespace in etc/mtree/BSD.include.dist - this file uses spaces for
  some reason.

Nice catch! Fixed.

My bad, removed. :)

I'm not familiar with H-VPLS, I assumed that 3 operations would be
enough per route. We're talking about the number of operations per
route entry and not label stack depth, right? Anyway, it should be
easily converted to use a tailq or similar structure. I'll keep HVPLS
in mind for future updates, thanks!

nuno

Committed.

http://leaf.dragonflybsd.org/mailarchive/commits/2008-07/msg00060.html