Bug #1038

MPLS support for DragonFly

Added by nant almost 6 years ago. Updated almost 6 years ago.

Status:ClosedStart date:
Priority:LowDue date:
Assignee:nant% Done:

0%

Category:-
Target version:-

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

History

#1 Updated by dillon almost 6 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
<>

#2 Updated by nant almost 6 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

#3 Updated by sepherosa almost 6 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

#4 Updated by nant almost 6 years ago

Sure, thanks! :)

Ok should i put it in before or after release?

nuno

#5 Updated by sepherosa almost 6 years ago

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

Best Regards,
sephe

#6 Updated by hasso almost 6 years ago

Me too.

#7 Updated by hasso almost 6 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.

#8 Updated by nant almost 6 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

#9 Updated by hsu2 almost 6 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;
}

#10 Updated by dillon almost 6 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
<>

#11 Updated by nant almost 6 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

#12 Updated by nant almost 6 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

Also available in: Atom PDF