Re: MPLS for Linux kernel

El 22/11/2011 14:55, Igor Maravić escribió:

2011/11/22 Jorge Boncompte [DTI2] [off-list ref]:

quoted

       You keep insisting in that you fixed a lot of things, but you have provided a
git tree with just one big commit and say that have taken some of my patches on
it, could you please provide patches on TOP of the sourceforge code for the
things that are not fixed there?

I insist on that because I did do a lot of things. When I did send you
patch, on TOP of your net-next code, about the most important bug
(stack overflow that was happening when mpls nhlfe entry was built)
that I fixed it was just ignored. Unfortunately I started  fixing MPLS
code without git, and I added your patches manually.

	(Dropped Dave from the cc: list)

	But you failed to provide patches, that fix ONE thing at a time, and not a
patch with all the work that you have done, that's what you sent me for the
stack overflow bug or have done now with the git tree. Providing clean,
separated patches it's YOUR work if you WANT to see them applied on the
mpls-linux tree.

quoted

       The kernel code that is not commented out on the mpls-linux code when you build
the kernel it the shim layer and it's not done on purpose. This code was written
by James to be a generic feature of the networking layer. Now I am not sure that
it has any value keeping it and am for removing it.

I didn't understand what did You want to say here.

	You have #ifdefed the MPLS code in the core networking code, that's wrong,
that's not the way to go if you want to see the code merged upstream. The shim
layer was thought as a core component of the kernel. If we rip it what we come
at with should be #ifdef-less.

quoted

       The other thing that probably I am going to remove is the labelspace support. I
don't see a use for it, and even Cisco doesn't implement it either that I know.

That's 15 min of work, but I think that labelspaces should stay.

	Yes, I dit it an hour ago on a private branch. :) Why should did it stay?

quoted

       Then we must rework the netlink interface to make it cleaner and extensible.

What do you mean by extensible? With my netlink code you can add,
change and delete ilm, nhlfe and xc entries without any problem. What
other could one wish for? As I could see in your code you can't change
ilm, nhlfe and xc entries.

	Yes, I did not finish the change code because no ones uses it currently
(iproute, quagga). In my opinion the instructions should be nested attributes,
and we have to change how the MPLS_CHANGE_* flags get passed, currently it's a hack.

quoted

       Check the dst's usages, there has been a lot of changes in the core kernel here
lately and I am not sure if we are using it correctly.

As far I could see you did a great job of using nhlfe as dst entry.
Problem was when you delete nhlfe entry that is referenced with ilm
entry. It shouldn't be allowed to be deleted, or the ilm should change
last instructions from fwd to peek. I did the last thing.

	If I delete an nhlfe I for sure don't want the traffic for be routed instead of
dropped. Instructions are policy set by the user and you shouldn't change them
under the hood.

Also there was the problem with neighbor hh_cache, because we are
using nhlfe as dst_entry but I fixed that too.  (hh_cache wouldn't
change when we are sending packets with diferent type (ETH_P_IP and
ETH_P_MPLS_UC))

	patch.

Also ilm shouldn't be dst_entry. I'm going to change that.

	I agree 100%. I did forgot it.

	Regards,
		Jorge