On Thu, Jul 22, 2021 at 12:14:23PM -0700, Ben Pfaff wrote: > 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 <[email protected]> > > Thanks a lot! I'll recheck this and push it soon.
I applied this to master. Grayson, do you want this backported? What version? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
