On 3 Oct 2023, at 18:14, Mike Pattrick wrote:
> On Mon, Oct 2, 2023 at 6:07 AM Simon Horman <[email protected]> wrote: >> >> On Fri, Sep 08, 2023 at 12:28:24PM -0400, Mike Pattrick wrote: >>> Currently a bridge mirror will collect all packets and tools like >>> ovs-tcpdump can apply additional filters after they have already been >>> duplicated by vswitchd. This can result in inefficient collection. >>> >>> This patch adds support to apply pre-selection to bridge mirrors, which >>> can limit which packets are mirrored based on flow metadata. This >>> significantly improves overall vswitchd performance during mirroring if >>> only a subset of traffic is required. >>> >>> I benchmarked this with two setups. A netlink based test with two veth >>> pairs connected to a single bridge, and a netdev based test involving a >>> mix of DPDK nics, and netdev-linux interfaces. Both tests involved >>> saturating the link with iperf3 and then sending an icmp ping every >>> second. I then measured the throughput on the link with no mirroring, >>> icmp pre-selected mirroring, and full mirroring. The results, below, >>> indicate a significant reduction to impact of mirroring when only a >>> subset of the traffic on a port is selected for collection. >>> >>> Test No Mirror | No Filter | Filter | No Filter | Filter | >>> +-----------+-----------+-----------+-----------+----------+ >>> netlink | 39.0 Gbps | 36.1 Gbps | 38.2 Gbps | 7% | 2% | >>> netdev | 7.39 Gbps | 4.95 Gbps | 6.24 Gbps | 33% | 15% | >>> >>> The ratios above are the percent reduction in total throughput when >>> mirroring is used either with or without a filter. >>> >>> Signed-off-by: Mike Pattrick <[email protected]> >> >> ... >> >>> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c >> >> ... >> >>> @@ -212,7 +218,9 @@ mirror_set(struct mbridge *mbridge, void *aux, const >>> char *name, >>> struct ofbundle **dsts, size_t n_dsts, >>> unsigned long *src_vlans, struct ofbundle *out_bundle, >>> uint16_t snaplen, >>> - uint16_t out_vlan) >>> + uint16_t out_vlan, >>> + const char *filter, >>> + const struct ofproto *ofproto) >> >> Hi Mike, >> >> I am concerned about the implications of passing large numbers of >> function arguments, which seems to happen several places in this patch. > > Hi Simon, > > I agree, I'll send in a new patch to reduce the number of arguments we > pass in these functions. I think pre-patch we already exceeded a > reasonable number of parameters for these functions anyways. Will wait for the new rev, and do a review. > Thank you, > Mike > >> >>> { >>> struct mbundle *mbundle, *out; >>> mirror_mask_t mirror_bit; >> >> ... >> >>> @@ -430,6 +482,7 @@ mirror_get(struct mbridge *mbridge, int index, const >>> unsigned long **vlans, >>> if (!mirror) { >>> return false; >>> } >>> + >>> /* Assume 'mirror' is RCU protected, i.e., it will not be freed until >>> this >>> * thread quiesces. */ >>> >> >> nit: this hunk seems unrelated to the subject of this patch. >> >> ... >> > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
