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...