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.