On Sat, May 17, 2014 at 09:39:10AM +0900, Simon Horman wrote: > On Fri, May 16, 2014 at 08:55:12AM -0700, Ben Pfaff wrote: > > On Fri, May 16, 2014 at 11:30:23AM +0900, Simon Horman wrote: > > > In some cases an pop MPLS action changes a packet to be a non-mpls packet. > > > In this case subsequent any L3+ actions require access to portions > > > of the packet which were not decoded as they were opaque when the > > > packet was MPLS. Allow such actions to be translated by > > > first recirculating the packet. > > > > > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > > > The first three patches look fine to me. > > > > compose_recirculate_action() in this one makes me worry a bit about a > > corner case. If there is no rule, then compose_recirculate_action() > > allocates one. I think that in the common case, there is a rule. But a > > "packet_out" message does translate actions without a rule. I think > > that means that each "packet_out" from a controller can create > > unnecessarily create a recirculation id and corresponding rule that will > > take a long time to expire and never get reused. > > > > If I'm right about that, then one solution would be to do recirculation > > in userspace in the case where we have a packet and no rule. That's a > > pretty nasty corner case, though (do we ever want to do that in other > > circumstances?) and so for now I'd be happy with a comment in the > > ofproto_dpif_alloc_recirc_id() case in compose_recirculate_action() > > explaining what's going on. > > I believe your analysis is correct and in fact in an earlier version of the > patchset I did handle this case by recirculating in user-space. That was > done by executing actions in userspace which also tried to solve some the > problem of recirculation for in_ports that don't exist in the datapath. The > outcome of the resulting conversation was that for the latter problem at > least executing in userspace isn't sufficiently generic as some actions > (hash) can't be performed in userspace. And that problem is being > addressed separately (though no patches so far that I am aware of). > > Back to the problem at hand, it probably could be addressed by > executing actions in userspace. So long as we could convince ourselves > that the hash action would never show up (probably true). But it is > a rather more complex solution than the one included in this patch. > > I do think this is a corner case (though I could well be wrong). Assuming > that is the case I don't think the complexity of recirculating in userspace > is warranted and I would rather take your offer to put a comment in > ofproto_dpif_alloc_recirc_id(). > > > I think that, for now, recirculation will lose metadata fields like > > registers. That's not a new problem, and I believe that Andy Zhou (or > > maybe Justin Pettit?) is working on a general solution, so I'm not > > asking you to fix it, but I think that it also bears mention in a > > comment. > > Thanks. I had noticed that at some point but it slipped my mind as > I didn't have a user case. I'll add a comment as you suggest. > > > I don't understand may_xlate_l3_actions. It seems to cover a lot more > > than L3, e.g. VLANs and Goto-Table. > > It originally had a more limited use case which has grown. > I'll check over the code with a view to making it clearer.
Thanks for the responses, Simon. I look forward to the next version. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
