On Mon, Jan 9, 2017 at 2:55 PM, Mickey Spiegel <[email protected]> wrote:
> > On Mon, Jan 9, 2017 at 2:44 PM, Ben Pfaff <[email protected]> wrote: > >> On Mon, Jan 09, 2017 at 02:30:54PM -0800, Mickey Spiegel wrote: >> > On Mon, Jan 9, 2017 at 2:22 PM, Ben Pfaff <[email protected]> wrote: >> > >> > > On Fri, Jan 06, 2017 at 04:28:00PM -0800, Mickey Spiegel wrote: >> > > > On Fri, Jan 6, 2017 at 3:57 PM, Ben Pfaff <[email protected]> wrote: >> > > > >> > > > > On Fri, Jan 06, 2017 at 12:00:31PM -0800, Mickey Spiegel wrote: >> > > > > > This patch adds the capability to force loopback at the end of >> the >> > > > > > egress pipeline. A new flags.force_egress_loopback symbol is >> > > defined, >> > > > > > along with corresponding flags bits. When >> > > flags.force_egress_loopback >> > > > > > is set, at OFTABLE_LOG_TO_PHY, instead of the packet being sent >> out >> > > to >> > > > > > the peer patch port or out the outport, the packet is forced >> back to >> > > > > > the beginning of the ingress pipeline with inport = outport. >> All >> > > > > > other registers are cleared, as if the packet just arrived on >> that >> > > > > > inport. >> > > > > > >> > > > > > This capability is needed in order to implement some of the >> east/west >> > > > > > distributed NAT flows. >> > > > > > >> > > > > > Note: The existing flags.loopback allows a packet to go from >> the end >> > > > > > of the ingress pipeline to the beginning of the egress pipeline >> with >> > > > > > outport = inport, which is different. >> > > > > > >> > > > > > Initially, there are no tests incorporated in this patch. This >> > > > > > functionality is tested in a subsequent distributed NAT flows >> patch. >> > > > > > Tests specific to egress loopback may be added once the >> capability >> > > > > > to inject a packet with one of the flags bits set is added. >> > > > > > >> > > > > > Signed-off-by: Mickey Spiegel <[email protected]> >> > > > > >> > > > > I don't really understand this yet. >> > > > > >> > > > > Does this need to be a flag or can it be an action, i.e. one that >> > > > > immediately jumps back to the beginning of the ingress pipeline. >> Then >> > > > > we don't need hard-coded flags, we can just have used-defined >> register >> > > > > bits, etc. >> > > > > >> > > > >> > > > Since I am figuring out whether to do egress loopback at the end of >> > > > the egress pipeline, I could get rid of the FORCE_EGRESS_LOOPBACK >> > > > flag and use an action instead. >> > > >> > > OK. >> > > >> > > > I think I still need the EGRESS_LOOPBACK_OCCURRED bit to avoid >> > > > the packet getting dropped in table 1 because the logical router >> receives >> > > > a packet with its own IP address as source. >> > > >> > > I think that could be avoided, too, with a little more adjustment. >> > > First, instead of zeroing all the registers, maintain them (and then >> > > zero registers that should be zeroed using OVN logical actions). >> > > Second, use some designated bit in a register for this particular >> > > purpose. >> > > >> > > (In case it is not clear, my preference, overall, is to put policy, as >> > > much as possible, into the logical flow table instead of into the >> > > mechanism that surrounds it.) >> > > >> > >> > Probably the register bit setting should be within the clone. >> > Are you OK with setting a specific register bit in an OVN action >> definition? >> >> What do you mean by "within the clone"? >> >> To clarify what I'm talking about, I was expecting something like this >> to appear in the OVN logical actions: >> reg0=0; reg1=0; reg2=0; ...; regX[Y] = 1; recirculate; >> Maybe we need to save and restore the registers around the >> recirculation, in which case we can define a new OVN logical action that >> does that, e.g. maybe an OVN clone action: >> clone { reg0=0; reg1=0; reg2=0; ...; regX[Y] = 1; recirculate; } >> > > I was thinking a single egress_loopback action that took care of the > clone, setting inport = outport, clearing the registers, and resubmitting > to table 16 all under the covers. > > Should ovn-northd really be coding "reg0=0; reg1=0; ...; reg9=0"? > Investigating the details a little further, a couple of issues come up: - recirculate vs egress_loopback While recirculate seems more general, resetting the ct_zones becomes a problem. The ct_zones are not known to ovn-northd. For a general purpose recirculate to any logical inport, the ovn actions would require the equivalent of ovn/controller/physical.c get_zone_ids(). I do see ep.ct_zones in ovn/controller/lflow.c which is passed to ovn/lib/actions.c, but I do not see any code that actually uses that at the moment. While this could be implemented, if I can restrict the semantics to egress_loopback (i.e. inport = outport) then I do not need to change any of the three zones, so that complexity can be avoided. - in_port If egress loopback is implemented in ovn/controller/physical.c, then I can set the in_port. If it is implemented as an OVN action, then the simple answer is to just set it to zero. My use case is for a patch port, in which the value will be zero anyways. So, my suggestion is: egress_loopback { regX[Y] = 1 } This would trigger the following actions under the covers: size_t clone_ofs = ofpacts_p->size; struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p); put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p); put_move(MFF_LOG_OUTPORT, 0, MFF_LOG_INPORT, 0, 32, ofpacts_p); put_load(0, MFF_LOG_OUTPORT, 0, 32, ofpacts_p); for (int i = 0; i < MFF_N_LOG_REGS; i++) { put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p); } /* Actions from between { }. */ put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone); ofpacts_p->header = clone; ofpact_finish_CLONE(ofpacts_p, &clone); Mickey _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
