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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
