Re: [mpls-linux-devel] Re: 2.6 Spec: Random comments.

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

Comments in line

On Fri, Feb 13, 2004 at 09:09:08AM -0500, Jamal Hadi Salim wrote:
> 
> Maybe we can move this discussion to the list and leave
> Dave alone; we can ping him when we need to verify things from him.
> I am trying to cc the list as a test.
> 
> On Fri, 2004-02-13 at 06:26, Ramon Casellas wrote:
> > Some comments,
> > 
> > (I'm still reading the spec, and slowly looking at the main entry points
> > and hooks in the code, so please be patient and bear with me)
> > 
> > 
> > RCAS 20040213: I see the utility of FEC Id, but I am not fond of the name.
> > The name Fec Id implies that it is "a FEC identifier". 
> 
> Essentially it is an identifier of a NHLFE entry. 
> So you are right naming it a FEC identifier may not be the best.

Agreed.

>  
> > What worries me is
> > the mapping FECId -> NHLFE (for example, in LSP merging, two FECids could
> > be mapped to the same NHLFE index), and the fact that a FECId should be a
> > member of a NHLFE entry...
> 
> I dont wanna call the so-far-called fecid lspid but it is close.
> 
> > Moreover, core LSRs should be FEC agnostic. This was my main comment last
> > time. Basically, the FECid is the label itself. The label implicitely
> > identifies the FEC as is the "key" to use to forward the packet. Otherwise 
> > you have at the same time label mappings and fecid management (signalling 
> > protocols)
> 
> I wouldnt call it a label at all. It is the key used to search the
> NHLFE. Some implementations dont allow setting of such a parameter (one
> of the vendors i looked at did actually) - they will tell you what it
> is. Essentially it is an identifier of a NHLFE entry (not index). 
> A collection of these NHLFE entries could be used by the same LSP.
> There is a further entry that can be used to store extra LSP info
> (refer to parameter "index")

> Given the above info, suggest a new name. Maybe NHid?

Much better then FECid ;-) (although it is just a name ...)

> > "ILM and FTN derive a FECid from their respective lookups"
> > 
> > I would propose :  "ILM and FTN derive a [list of - multipath] NHLFE index
> > [es] from their respective lookups [...] These indexes and incoming
> > labelspaces are then used to lookup the NHLFE to determine how to forward
> > the packet."
> > 
> 
> Ok, that is useful. I have not tested multipath but it should
> work with Linux routing ECMP at least. 
> I wouldnt call it NHLFE indices rather these identifiers so far
> called fecid;
> Also i would think most of these lists would contain a single entry.
> BTW, the ILM is not multihop ready. We should be able to add easily.
> Also there is no controil on how the multihop selection is done with
> the linux routing table - whatever Linux ECMP does goes.
> We should be able to fix the ILM with an algorith selector.

After looking at the code I would agreed that whatever linux multiple does
at the ingress LER, this code will follow.  The real question is how to
go about supporting multipath as an LSR? (one ILM needs to load balance over
multiple NHLFE).  Or dare I suggest p-mp LSPs?

> > A standard structure for NHLFE contains:
> > - FEC id 
> > 
> > RCAS: Is this field really necessary)? a NHLFE entry could be shared by
> > several 'FECsId'...
> > 
> 
> Look at my description above.
> The ability to select this value by policy allows us to be able to
> select the NHLFE entries from other subsystems; eg a u32 classifier
> on ingress could select all IP addresses from 10.1.1.1/24 to have a
> fecid of 10. The skb->fecid is then set to 10. When the packet gets to
> the point of NHLFE entry selection this value is used to override/select
> the NHLFE entry.

I know you mentioned it is "not an index" but to me it seems like it really
_is_ an index for the NHLFE. Can multiple NHids correspond to the same NHLFE?
If it is a 1 to 1 mapping for all intents an purposes it is an index :-)

> > And a couple of questions (please consider them as questions from someone
> > who has limited experience in kernel programming)
> > 
> > * I think that adding struct mpls_nhlfe_route *mpls to a dst_entry is a
> >   little intrusive, and somehow the "genericity" of the DST is being lost.
> >   Would not it be better to use :
> > 
> > struct mpls_dst 
> > {
> > 	union
> > 	{
> > 		struct dst_entry        dst;
> > 		struct mpls_dst        *md_next;
> > 	} u;
> > 	....
> > 
> > and manage MPLS dsts from the mpls subsystem? I understand that using your
> > approach it is easier to get MPLS information from skb->dst->mpls but I
> > don't know, it seems a too strong coupling between MPLS and generic dst
> > management. Well, just food for thoughts.
> 
> 
> dsts are still managed from the MPLS code. There is some generic stuff
> (create, destriy, gc etc) for which there is no point in recreating in
> the MPLS code
> The way it is right now works fine. What could probably have been a
> better approach is to stack dsts. It would require some surgery and i am
> not sure i have the patience for it. Mayeb we can ask Dave on his
> thoughts on this.

Currently we use dst stacking.  The 'child' dst is actually a static member
of the 'out going label info' (NHLFE).  So when the skb reaches the the exit
of IPv4|6 a check for the child is done.  The skb->dst is replaced with the
child dst and the child output funtion is called (which sends it into
MPLS land).  The entrace to MPLS land use "container_of" macro to get the
NHLFE to used to forward the packet.  How the stacked dst is created is
similar to your scheme.  I was wondering is XFRM is a better scheme to use
for all of this?

> cheers,
> jamal
> 
> 
> 
> -------------------------------------------------------
> 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...