On Thu, Jul 5, 2012 at 7:48 AM, ravi kerur <rke...@gmail.com> wrote: > I understand code review is time consuming and please note it's a bit time > consuming to me as well as I have to retest/rebase everytime and I am not > sure whether the amount of time I am spending on this is justifiable?
Reviewing this code is frustrating because you don't really take into account the comments that I make. There are changes along the lines that I ask but they generally don't address the heart of the matter. Until that changes, this isn't really a good use of either of our time. I would focus less on writing and sending out new code and more time on really understanding the problems at hand. This isn't just in relationship to the comments that I've made - your upstream patch for emulation of offloading doesn't even follow the pattern of existing code (for example, how do you know when to emulate?). > MPLS actions are implemented as per section 4.8 in OF 1.1 specification, > excerpts below > > "The Apply-Actions instruction and the Packet-out message include an action > list. The semantic of the > action list is identical to the OpenFlow 1.0 specification. The actions of > an action list are executed in the > order specified by the list, and are applied immediately to the packet." As I mentioned before, we're talking about the kernel API, not the OpenFlow one. > I looked at implementing ttl actions as set operations and didn't find it > suitable as every ttl action is applied immediately and it won't integrate > well with other mpls actions(push/pop and set label) which are applied > immediately. Please note there can be more than 2 mpls push and pop actions > configured. Supporting additional levels of labels is a perfect use case for recirculation. I hope you realize by now that I'm not going to accept a patch even if it implements the immediate use case and is expedient. It needs to be well designed with a reasonable effort at thinking about how it might be used in the future. If the proper way to implement something is through recirculation then you must implement recirculation. Since the API you are defining is permanent you can't do something else now and hope that someone else fixes it in the future. That being said, I'm fine with not implementing certain portions now because that leaves us free to come back to it later once all of the pieces are there. That's why I suggested that you implement these actions purely in userspace. Partial MPLS or slow MPLS is better than no MPLS as long as we can improve it in the future. > With respect to packet recycling I thought it was for handling layer-3 after > executing mpls actions or could be generic purpose as well but focused on > inner-header parsing or further actions executed based on inner-header after > applying actions to outer-header. Packet recycling for ttl operations looks > like an overkill to me as you might be aware that packet recycling has > performance impact. Do you actually know what the performance impact is or are you just speculating? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
