Project

General

Profile

Actions

Bug #1038

closed

MPLS support for DragonFly

Added by nant over 15 years ago. Updated over 15 years ago.

Status:
Closed
Priority:
Low
Assignee:
Category:
-
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

Description

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

Actions #1

Updated by dillon over 15 years ago

: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
<>
Actions #2

Updated by nant over 15 years ago

On Tue, Jul 1, 2008 at 11:57 PM, Matthew Dillon
<> 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
<>

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

Actions #3

Updated by sepherosa over 15 years ago

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

Actions #4

Updated by nant over 15 years ago

Sure, thanks! :)

Ok should i put it in before or after release?

nuno

Actions #5

Updated by sepherosa over 15 years ago

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

Best Regards,
sephe

Actions #6

Updated by hasso over 15 years ago

Me too.

Actions #7

Updated by hasso over 15 years ago

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.
Actions #8

Updated by nant over 15 years ago

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

Actions #9

Updated by hsu2 over 15 years ago

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

Actions #10

Updated by dillon over 15 years ago

:
: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
&lt;&gt;
Actions #11

Updated by nant over 15 years ago

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

Actions #12

Updated by nant over 15 years ago

On Wed, Jul 2, 2008 at 10:15 AM, Hasso Tepper
<> wrote:

Hasso Tepper <> 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

Actions

Also available in: Atom PDF