[ovs-dev] [PATCH v2 1/5] vswitchd: Extend vswitchd schema for mirror HWOL

[Wang, Liang-min]

> -----Original Message-----
> From: Miskell, Timothy <timothy.miskell at intel.com>
> Sent: Wednesday, May 26, 2021 10:05 AM
> To: Ben Pfaff <blp at ovn.org>
> Cc: dev at openvswitch.org; maxime.coquelin at redhat.com; Wang, Liang-min
> <liang-min.wang at intel.com>
> 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 <liang-min.wang at intel.com
> > Tested-by: Timothy Miskell <timothy.miskell at intel.com>
> > Suggested-by: Munish Mehan <mm6021 at att.com>
> 
> 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.

> Here are some more detailed comments.
> 
> It's great to give examples in the commit message.  It would be nice to put
> these examples somewhere in the documentation.  For example, you could
> add them to the existing Port Mirroring examples in ovs-vsctl(8).
> 
> The code here looks pretty preliminary.  This isn't a full review of it.
> I see that it has log messages that don't seem final.  It also seems like the
> person who wrote it didn't read the coding style document based on stuff
> like this:
> 
> > +    if (info->src) {
> > +        free(info->src);
> > +    }
> 
> And this is just weird, what's wrong with xstrdup()?  I see this pattern
> multiple times:
> > +    if (m->name) {
> > +        info->name = xmalloc(strlen(m->name)+1);
> > +        ovs_strzcpy(info->name, m->name, strlen(m->name));
> > +    }
> 
> This leaks memory:
> > +        if (info->n_src_port != cfg->n_output_src_vlan) {
> > +            VLOG_ERR("src port count:%d ouput src vlan count:%ld",
> > +                info->n_src_port, cfg->n_output_src_vlan);
> > +            return -1;
> > +        }
> 
> The documentation updates to vswitch.xml are in the "Selecting Packets for
> Mirroring" section but most of them are not about selecting packets.
> I guess there should be a new "Mirror Offload" section.
> 
> Also I don't know what a "BDF string" is (BDF is a type of bitmap font).
> BFD, maybe?  The format of the string should be clearly defined instead of
> just saying that it's some kind of string.
> 
> The flow_src_mac and flow_dst_mac specifications don't make sense
> because OSVDB columns are unordered sets so there is no way to pair them
> up.  They would be better as maps from port names to MAC addresses.
> 
> The documentation misspells "parameter" in many places.  But these aren't
> parameters, they're columns or, more abstractly, features.
> 
> I don't see why output_src_vlan and output_dst_vlan both exist.  There isn't
> currently any distinction between packets that are selected for one reason or
> another.  It's going to cause confusion to introduce one.