Message-ID: <4ECBB2C4.9000505@dti2.net>
Date:	Tue, 22 Nov 2011 15:33:40 +0100
From:	"Jorge Boncompte [DTI2]" <jorge@...2.net>
To:	igorm@....rs
CC:	netdev@...r.kernel.org
Subject: Re: MPLS for Linux kernel

El 22/11/2011 14:55, Igor Maravić escribió:
> 2011/11/22 Jorge Boncompte [DTI2] <jorge@...2.net>:
>>        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.

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

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

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

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists