mpls-linux-devel

[mpls-linux-devel] DaveM - mpls_nhlfe_hash_code()

[James R. L. <jl...@mi...>]

I see a minor flaw we need to fix in the DaveM code.

If I read the code correctly, the hash for the NHLFE is
calculated using the out-going label and the out-going ifindex.
If this is the case, then we get an unresolvable collision when 2
or more peers off of the same interface allocate the same label
(which is perfectly valid).  Either we can add the nexthop address
as part of the search criteria (yuck!) or just aknowledge that
NHLFE's have a unique index which is not detemined by the contents of
it.  The second fits well with the LSR MIB in which the out-segments
have an arbitrary unique index.

What do you guys think?

-- 
James R. Leu
jl...@mi...

Re: [mpls-linux-devel] DaveM - mpls_nhlfe_hash_code()

[Ramon C. <cas...@in...>]

Morning,



On Thu, 19 Feb 2004, James R. Leu wrote:

> I see a minor flaw we need to fix in the DaveM code.
> If I read the code correctly, the hash for the NHLFE is
> calculated using the out-going label and the out-going ifindex.

(Unless I'm on crack, the hash is computed using only
the label) I think you mean actually the list walk, where each list entry
is compared to the nhlfe_id and the ifindex.


> If this is the case, then we get an unresolvable collision when 2
> or more peers off of the same interface allocate the same label
> (which is perfectly valid).

Indeed.


> Either we can add the nexthop address
> as part of the search criteria (yuck!) or just aknowledge that

IMHO it is not needed.


> NHLFE's have a unique index which is not detemined by the contents of
> it.  The second fits well with the LSR MIB in which the out-segments
> have an arbitrary unique index.

Right, I see your point... I always assumed that. (Did I imply the
contrary on any of my prev emails?)  Unless I'm wrong, for me the nhlfe_id
is a unique identifier (I called them indexes once ;)) The user is aware
of the contents of the nhlfe when she sets the mappings, but thats all.

In summary, if I understand you correcly, code should be:

        list_for_each_entry(walk, &mpls_nhlfe_hash[hash], mr_hash) {
                if (walk->mr_nhlfeid   == nhlfeid)
                         r = walk;

and two more things:
* either the nhlfeids are _globally_ unique (in that case we don't need
the space argument) or
* nhlfeids are unique in a given labelspace if ((..) && space ==

my vote: the oif should not be used as a key for nhlfe lookup. Moreover...
the current code (although correct since it is the user that sets that) is
kinda strange... given that IIRC the rfc states that the neighbour (and
subsequently the oif) should be obtained from the NHLF table (not be used
to look it up), and not be IGP constrained. Anyway, I assume it is an
implementation detail and that it is more convenient to have it in the
FIB_RES (in out_dev) and that in any case the actual neighbour is held by
the nhlfe and it is bound using mpls_bind_neighbour.(I liked
more the idea of having the NH and the oif in the nhlfe itself more
concretely, in the set opcode)....

Another question: why is the struct postfixed with _route? what's the
point of calling it mpls_nhlfe_route and not plain mpls_nhlfe?


Thanks,
R.

Re: [mpls-linux-devel] DaveM - mpls_nhlfe_hash_code()

[James R. L. <jl...@mi...>]

Comments in line

On Fri, Feb 20, 2004 at 07:19:19AM +0100, Ramon Casellas wrote:
> 
> Morning,
> 
> 
> 
> On Thu, 19 Feb 2004, James R. Leu wrote:
> 
> > I see a minor flaw we need to fix in the DaveM code.
> > If I read the code correctly, the hash for the NHLFE is
> > calculated using the out-going label and the out-going ifindex.
> 
> (Unless I'm on crack, the hash is computed using only
> the label) I think you mean actually the list walk, where each list entry
> is compared to the nhlfe_id and the ifindex.
> 
> 
> > If this is the case, then we get an unresolvable collision when 2
> > or more peers off of the same interface allocate the same label
> > (which is perfectly valid).
> 
> Indeed.
> 
> 
> > Either we can add the nexthop address
> > as part of the search criteria (yuck!) or just aknowledge that
> 
> IMHO it is not needed.
> 
> 
> > NHLFE's have a unique index which is not detemined by the contents of
> > it.  The second fits well with the LSR MIB in which the out-segments
> > have an arbitrary unique index.
> 
> Right, I see your point... I always assumed that. (Did I imply the
> contrary on any of my prev emails?)  Unless I'm wrong, for me the nhlfe_id
> is a unique identifier (I called them indexes once ;)) The user is aware
> of the contents of the nhlfe when she sets the mappings, but thats all.
> 
> In summary, if I understand you correcly, code should be:
> 
>         list_for_each_entry(walk, &mpls_nhlfe_hash[hash], mr_hash) {
>                 if (walk->mr_nhlfeid   == nhlfeid)
>                          r = walk;

That and we should never build the hash based on the label to begin with.
This all goes back whether or not the label for a out-segment can change.
> 
> and two more things:
> * either the nhlfeids are _globally_ unique (in that case we don't need
> the space argument) or
> * nhlfeids are unique in a given labelspace if ((..) && space ==

Labelspace has no meaning when talking about NHLFE or out-segments.
Labelspaces are meant to help the LSR manage the labels it advertises,
in otherwords its ILM or in-segments.

I think nhlfeids should be globally unique.

> my vote: the oif should not be used as a key for nhlfe lookup. Moreover...
> the current code (although correct since it is the user that sets that) is
> kinda strange... given that IIRC the rfc states that the neighbour (and
> subsequently the oif) should be obtained from the NHLF table (not be used
> to look it up), and not be IGP constrained. Anyway, I assume it is an
> implementation detail and that it is more convenient to have it in the
> FIB_RES (in out_dev) and that in any case the actual neighbour is held by
> the nhlfe and it is bound using mpls_bind_neighbour.(I liked
> more the idea of having the NH and the oif in the nhlfe itself more
> concretely, in the set opcode)....

That is fine if every out-segment is responsible for sending
the PDU (ie needs the oif and neighbour), but if you build hiearchy via
indirection, then there are out-segments which do not actually send the PDU,
they just hand it off to another out-segment to continue processing, thus 
they do not need a oif or neighbour.

> 
> Another question: why is the struct postfixed with _route? what's the
> point of calling it mpls_nhlfe_route and not plain mpls_nhlfe?

It is a side effect of an MPLS implementation that used IPv4 as a roadmap.

> 
> 
> Thanks,
> R.
> 
> 
> 
> 
> 
> 
> 
> -------------------------------------------------------
> SF.Net is sponsored by: Speed Start Your Linux Apps Now.
> Build and deploy apps & Web services for Linux with
> a free DVD software kit from IBM. Click Now!
> http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
> _______________________________________________
> mpls-linux-devel mailing list
> mpl...@li...
> https://lists.sourceforge.net/lists/listinfo/mpls-linux-devel

-- 
James R. Leu
jl...@mi...

Re: [mpls-linux-devel] DaveM - mpls_nhlfe_hash_code()

[Ramon C. <cas...@in...>]

On Fri, 20 Feb 2004, James R. Leu wrote:



> That and we should never build the hash based on the label to begin with.
> This all goes back whether or not the label for a out-segment can change.

I see. So the lookup basically reduces to the nhlfeid itself, which to me
seems fine.


> Labelspace has no meaning when talking about NHLFE or out-segments.
> Labelspaces are meant to help the LSR manage the labels it advertises,
> in otherwords its ILM or in-segments.

Agreed. We did not use labelspaces for moi objects.



>
> I think nhlfeids should be globally unique.

Agreed.


> That is fine if every out-segment is responsible for sending
> the PDU (ie needs the oif and neighbour), but if you build hiearchy via
> indirection, then there are out-segments which do not actually send the PDU,
> they just hand it off to another out-segment to continue processing, thus
> they do not need a oif or neighbour.

Well, I agree :) (I have been working with you before ;)) but what would
you suggest to do? Jamal? comments? I think I should start gathering a
list of potential changes + arguments, so we can later justify them to the
original author of the code... Otherwise, I wont be able to remember why
we change this or that


>
> >
> > Another question: why is the struct postfixed with _route? what's the
> > point of calling it mpls_nhlfe_route and not plain mpls_nhlfe?
>
> It is a side effect of an MPLS implementation that used IPv4 as a roadmap.

Ok, can be easily fixed :P
R.

Re: [mpls-linux-devel] DaveM - mpls_nhlfe_hash_code()

[James R. L. <jl...@mi...>]

Comments in line

On Fri, Feb 20, 2004 at 08:20:17AM +0100, Ramon Casellas wrote:
> On Fri, 20 Feb 2004, James R. Leu wrote:
> 
> 
> 
> > That and we should never build the hash based on the label to begin with.
> > This all goes back whether or not the label for a out-segment can change.
> 
> I see. So the lookup basically reduces to the nhlfeid itself, which to me
> seems fine.
> 
> 
> > Labelspace has no meaning when talking about NHLFE or out-segments.
> > Labelspaces are meant to help the LSR manage the labels it advertises,
> > in otherwords its ILM or in-segments.
> 
> Agreed. We did not use labelspaces for moi objects.
> 
> 
> 
> >
> > I think nhlfeids should be globally unique.
> 
> Agreed.
> 
> 
> > That is fine if every out-segment is responsible for sending
> > the PDU (ie needs the oif and neighbour), but if you build hiearchy via
> > indirection, then there are out-segments which do not actually send the PDU,
> > they just hand it off to another out-segment to continue processing, thus
> > they do not need a oif or neighbour.
> 
> Well, I agree :) (I have been working with you before ;)) but what would
> you suggest to do? Jamal? comments? I think I should start gathering a
> list of potential changes + arguments, so we can later justify them to the
> original author of the code... Otherwise, I wont be able to remember why
> we change this or that

I think that is a fine idea.  Lets only change what we absolutly
have to, but note why other more significant changes should be made.

> 
> 
> >
> > >
> > > Another question: why is the struct postfixed with _route? what's the
> > > point of calling it mpls_nhlfe_route and not plain mpls_nhlfe?
> >
> > It is a side effect of an MPLS implementation that used IPv4 as a roadmap.
> 
> Ok, can be easily fixed :P

True
s/mpls_nhlfe_route/mpls_out_info^H^H^H^H^H^H^H^nhlfe/
;-)

> R.

-- 
James R. Leu
jl...@mi...