On Wed, May 26, 2021 at 04:03:00PM +0000, Wang, Liang-min wrote: > > -----Original Message----- > > From: Miskell, Timothy <[email protected]> > > Sent: Wednesday, May 26, 2021 10:05 AM > > To: Ben Pfaff <[email protected]> > > Cc: [email protected]; [email protected]; Wang, Liang-min > > <[email protected]> > > Subject: RE: [ovs-dev] [PATCH v2 1/5] vswitchd: Extend vswitchd schema for > > mirror HWOL > > > > Adding Larry as the original author to discuss a lot of the points mentioned > > below. > > > > On Tue, May 25, 2021 at 05:20:37PM +0000, Timothy Miskell wrote: > > > This patch extends the vswitchd schema to allow specifying settings > > > specific to mirror hardware offload. The settings include: > > > mirror-offload, flow-dst-addr, bond-port-other, mirror-tunnel-addr, > > > output-src-vlan, and output-dst-vlan. The mirror-offload setting is a > > > boolean variable that can be used to enable mirror offloading. The > > > flow-dst-addr variable allows for filtering of traffic to be mirrored > > > based on the destination MAC address. The mirror-tunnel-addr variable > > > enables mirroring to ports attached to bridges other than the traffic > > > source bridge. The output-src-vlan and the output-dst-vlan are used to > > > specify the VLAN tag for ingress and egress traffic. This allows a > > > VLAN tag to be added to mirrored traffic, which is used to trigger > > > device mirroring by means of the VLAN tag. The added VLAN tag can be > > > used to denote the source of the mirrored traffic. The added tag can > > > be stripped if the destination VNF has VLAN stripping enabled. > > > An example of configuring egress traffic mirroring with VLAN tag 100 > > > is as follows: > > > ovs-vsctl -- set bridge br-int mirror=@m1 -- --id=@vnic1 get port > > > vnic1 -- --id=@m1 create mirror name=egress mirror-offload=1 > > > select-src-port=@vnic1 mirror-tunnel-addr="0000\:87\:02.0" > > > output-src-vlan=100 > > > An example of configuring ingress traffic mirroring with VLAN tag 200 > > > is as follows: > > > ovs-vsctl -- set bridge br-int mirror=@m2 -- --id=@vnic1 get port > > > vnic1 -- --id=@m2 create mirror name=ingress mirror-offload=1 > > > select-dst-port=@vnic1 mirror-tunnel-addr="0000\:87\:02.1" > > > output-dst-vlan=200 > > > An example of configuring per-flow egress traffic mirroring with VLAN > > > tag > > > 300 is as follows: > > > ovs-vsctl -- set bridge br-int mirror=@m3 -- --id=@vnic1 get port > > > vnic1-- --id=@m3 create mirror name=perflow mirror-offload=1 > > > select-src-port=@vnic1 flow-dst-mac="52\:54\:00\:00\:00\:01" > > > mirror-tunnel-addr="0000\:18\:00.0" output-src-vlan=300 > > > Note: Users can use the default ovs-vsctl commands to inspect and > > > remove the mirror configuration settings. > > > > > > Signed-off-by: Liang-Min Wang <[email protected] > > > Tested-by: Timothy Miskell <[email protected]> > > > Suggested-by: Munish Mehan <[email protected]> > > > > Thanks for submitting a patch to improve OVS! > > > > We normally introduce hardware offload into OVS to take an OVS feature > > and make it faster by implementing it in hardware. I don't think this is > > that. It > > introduces a new feature that only works with particular hardware and > > particular restrictions, with an implementation that is different from the > > port > > mirroring that OVS has already: > > > > * With this commit, the destination of the mirroring is specified in a > > completely non-OVS way. It has to be directed to a particular piece > > of hardware in a way that only works if that destination is DPDK. > > > > * With this commit, lots of features only work if you are offloading > > (or possibly if you are using a DPDK device even if you are not > > offloading; I am not sure) but those features don't have any > > inherent dependency on offloading, it's just that the author didn't > > choose to implement them outside of offloading. > > > > I think that this is the wrong approach. I think this is what should be > > done: > > > > 1. In one commit or series of commits, add the features that this > > wants to add as features for port mirroring overall. No offload > > yet, only new features that work for every datapath. > > > > 2. A followup commit or commits add offload support. > > > > In particular, I don't think that it makes sense to add a new kind of mirror > > destination that is a DPDK-specific string. Just don't. Make the > > destination > > an OVS port. That port can point to a DPDK device. > > > > The existing OVS port mirroring is a VIRTIO-to-VIRTIO approach meaning the > source > traffic is from a VIRTIO port and the destination for the mirrored traffic is > also a > VIRTIO port. The performance impact of this type of traffic mirroring is well > known. This upstream is a new approach which is based upon a > VIRTIO-to-SR-IOV design meaning the source traffic is from a VIRTIO > port and the destination for the mirrored traffic is a SR-IOV > port. Support of device mirroring is not unique to one vendor. You are > right the initial commit is targeting at DPDK devices, and the same > technology is not limited to DPDK. We would like to work with > community to extend this technology for kernel VIRTIO port mirroring.
This is a mischaracterization. OVS port mirroring is from one OVS port to another. Offload in OVS works by taking something that OVS can already do and then implementing it more efficiently. This approach is backward: it takes something that hardware can do and then puts it into OVS in a hardware-specific manner. I am opposed to accepting the feature in this form. I reiterate my suggestion for an approach. Get the port mirroring features you want into OVS in a hardware-agnostic way. Then, add a way to offload those features. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
