[ovs-dev] [PATCH v5 4/7] ofproto-dpif: MPLS recirculation

[Ben Pfaff]

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