[ovs-dev] [PATCH] ofproto-dpif-xlate: Fix continuations with OF instructions in OF1.1+.

[Ben Pfaff]

On Wed, Jul 21, 2021 at 01:22:19PM +0200, Ilya Maximets wrote:
> On 7/21/21 12:52 AM, Ben Pfaff wrote:
> > On Tue, Jul 20, 2021 at 08:29:14PM +0200, Ilya Maximets wrote:
> >> On 7/7/21 8:51 PM, Ben Pfaff wrote:
> >>> +        /* From an OpenFlow point of view, goto_table and write_metadata are
> >>> +         * instructions, not actions.  This means that to use them, we'd have
> >>> +         * to reformulate the actions as instructions, which is possible, and
> >>> +         * we'd have slot them into the frozen actions in a specific order,
> >>> +         * which doesn't seem practical.  Instead, we translate these
> >>> +         * instructions into equivalent actions. */
[...]
> >> I have very little knowledge in this area, so this might be not issue.
> >> However, 'OpenFlow "instructions", which were introduced in OpenFlow 1.1'
> >> doesn't match with OFPACT_SET_FIELD that is marked as OpenFlow 1.2+
> >> action.  I see that all tests for continuation have default bridge
> >> configuration.  And monitors are OF10 and OF13 in different cases, but
> >> I'm not  sure if these ovs-ofctl monitors are checking that received
> >> actions for resume matches the OF version they are using.  And for
> >> the bridge itself, it supports all versions of OF, so it can interpret
> >> actions.  But the question is: will that work if the bridge only
> >> supports OpenFlow 1.1 ?  It should fail to parse OFPACT_SET_FIELD, or
> >> am I missing something here?
> > 
> > Thanks a lot for the review!
> > 
> > OVS can serialize most actions and instructions to most OpenFlow
> > versions, even if they are not natively supported in that particular
> > OpenFlow version.  For example, in the case of OFPACT_SET_FIELD (a
> > rather important action), the code in ofp-actions.c has complete
> > fallbacks:
[...]

> > What is lacking is a fallback for OF1.1+, which does have instructions,
> > in the case where we're encoding something that can't use the
> > instructions but must use actions instead.  That's the case here.  It's
> > a weird case and I don't know about it elsewhere.
> 
> OK.  Thanks for the explanation!  That make sense.  For some reason I
> mixed up ofpacts with OpenFlow actions in my head and so I missed the
> translation layer that handles conversion to the appropriate OpenFlow
> version.
> 
> TBH, it's really hard to track the call chain that connects the
> freeze_unroll_actions() and encode_SET_FIELD().  I still didn't figure
> that out by reading the code.  I guess, the only option for me is to
> attach debugger, break the execution and look at the stack, unless you
> can draw a call tree for me.

It's a little indirect.

If we're in freeze_unroll_actions(), it's because we're freezing.  If
we're freezing, we'll unwind the call stack a bit and end up in
finish_freezing__().  That function will stick the actions in a struct
frozen_state, and then attach it to a newly allocated recirc_id via
recirc_alloc_id_ctx().  That recirc_id gets stuck in an action that gets
appended to the flow we're currently translating.  In the case I'm
looking at here, that's an action to send the packet to userspace.

Later on, the datapath sends a packet sent to userspace.
process_upcall() in userspace hits the CONTROLLER_UPCALL case, which
grabs the recirc_id out of the upcall cookie, gets the frozen state
using that, gets the ofpacts out of it and stuffs them into the private
part of the packet-in message it constructs, and then sends an async msg
(a packet-in with private data attached) to the controller.  Eventually
that async message gets serialized using
ofputil_encode_packet_in_private(), which calls
ofputil_put_packet_in_private(), which encodes the actions via calls to
put_actions_property().

I wish it was more direct.  The real problem here is the fixed datapath
interface.  We can't change that unless and until we drop support for
the Linux kernel module and its frozen ABI.

> In the end, it's hard for me to review the logic behind this change,
> but the patch seems to do what described in the commit message and
> the implementation looks correct, tests work fine.  With that in mind:
> 
> Acked-by: Ilya Maximets <i.maximets at ovn.org>

Thanks a lot!  I'll recheck this and push it soon.