[ovs-dev] [PATCH] Extends the existing mirror configuration parameters

Wang, Liang-min liang-min.wang at intel.com
Wed May 19 11:53:53 UTC 2021


> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin at redhat.com>
> Sent: Wednesday, May 19, 2021 3:56 AM
> To: Wang, Liang-min <liang-min.wang at intel.com>; Miskell, Timothy
> <timothy.miskell at intel.com>; dev at openvswitch.org
> Subject: Re: [PATCH] Extends the existing mirror configuration parameters
> 
> Hi Liang-min,
> 
> When replying inline, please do not prefix with ">>" as it is handled as
> quoted text. There is no need to prefix.
> 
> On 5/18/21 8:00 PM, Wang, Liang-min wrote:
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin at redhat.com>
> >> Sent: Tuesday, May 18, 2021 12:15 PM
> >> To: Miskell, Timothy <timothy.miskell at intel.com>; dev at openvswitch.org
> >> Cc: Wang, Liang-min <liang-min.wang at intel.com>
> >> Subject: Re: [PATCH] Extends the existing mirror configuration
> parameters
> >>
> >> Hi Timothy, Liang-min,
> >>
> >> Thanks for rebasing the patch.
> >> A list of delta against the first RFC could help the reviewers.
> >> I notice one change in the right direction is the conversion to Vhost
> >> API datapath instead of Vhost PMD.
> >>
> >>> In this patch we support both Vhost API and Vhost PMD because OVS
> supports both
> >>> VhostUser and Vdev ports.
> >>>
> >> Also, I would suggest to have the patch split in several incremental
> >> patches to ease the review.
> >>
> >>> Thank you for suggestion. We will provide incremental patches on next
> submission
> >>>
> >> On 5/10/21 6:00 PM, Timothy Miskell wrote:
> >>> From: Liang-min Wang <liang-min.wang at intel.com>
> >>>
> >>> The following parameters are added:
> >>>  - mirror-offload: to turn on/off mirror offloading.
> >>>  - output-port-name: specify a port, using name string, that is on a
> different
> >>>    bridge
> >>>  - output-src-vlan: output port vlan for each select-src-port.
> >>>  - output-dst-vlan: output port vlan for each select-dst-port.
> >>>  - flow-src-mac: use src mac address of each select-dst-port for the
> header
> >>>    scan.
> >>>  - flow-dst-mac: use dst mac address of each select-src-port for the
> header
> >>>    scan.
> >>>  - mirror-tunnel-addr: BDF string of the tunnel device.
> >>>
> >>> ovs-vsctl test change because new mirroring parameters are introduced
> in
> >> this patch
> >>
> >> It would help to provide examples of usage of these new parameters.
> >>
> >>> Will add examples in the new patches
> >>>
> >>> Create a defer procedure call thread to handle all mirror offload
> requests.
> >>> This is a light-weight thread which remains in sleep-state when there is
> no
> >> new request.
> >>> This is created between ovs-vsctl and mirror offloading back end
> >>>
> >>> Implementing DPDK tx-burst (VIRTIO ingress traffic
> >>> mirror) and rx-burst (VIRTIO egress traffic mirror) callbacks.
> >>> Each callback  functions implement the following tasks:
> >>>  1. Enable per-packet VLAN insertion
> >>>    - for port mirroring, all packets are enabled per-packet VLAN insertion.
> >>>    - for flow mirroring, only packet header matches the required mac
> >> address
> >>>      are enabled.
> >>>  2. Sending the packets to the specified transport port (output-port in
> >>>     mirror offload configuration)
> >>>    - for port mirroring, all packets are sent to the transport port.
> >>>    - for flow mirroring, only matched packets are sent.
> >>>  3. Restore each packet attributes (remove DPDK per-packet offload
> flag)
> >>
> >> I will for sure have more questions later, but please find a few
> >> comments/questions below:
> >>
> >>> 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>
> >>> ---
> >>>  lib/automake.mk            |   2 +
> >>>  lib/netdev-dpdk-mirror.c   | 516
> >> +++++++++++++++++++++++++++++++++++++
> >>>  lib/netdev-dpdk-mirror.h   |  83 ++++++
> >>>  lib/netdev-dpdk.c          | 397 ++++++++++++++++++++++++++++
> >>>  lib/netdev-provider.h      |  16 ++
> >>>  lib/netdev.c               | 386 +++++++++++++++++++++++++++
> >>>  lib/netdev.h               |  16 ++
> >>>  tests/ovs-vsctl.at         |   2 +
> >>>  vswitchd/bridge.c          | 271 ++++++++++++++++++-
> >>>  vswitchd/vswitch.ovsschema |  24 +-
> >>>  vswitchd/vswitch.xml       |  50 ++++
> >>>  11 files changed, 1759 insertions(+), 4 deletions(-)
> >>>  create mode 100644 lib/netdev-dpdk-mirror.c
> >>>  create mode 100644 lib/netdev-dpdk-mirror.h
> >>>
> 
> ...
> 
> >>> +
> >>> +/* port/flow mirror traffic processors */
> >>> +static inline uint16_t
> >>> +netdev_custom_mirror_offload_cb(uint16_t qidx, struct rte_mbuf
> >> **pkts,
> >>> +    uint16_t nb_pkts, void *user_params)
> >>> +{
> >>> +    struct mirror_param *data = user_params;
> >>> +    uint16_t i, dst_qidx, match_count = 0;
> >>> +    uint16_t pkt_trans;
> >>> +    uint16_t dst_port_id = data->dst_port_id;
> >>> +    uint16_t dst_vlan_id = data->dst_vlan_id;
> >>> +    struct rte_mbuf **pkt_buf = &data->pkt_buf[qidx * data-
> >>> max_burst_size];
> >>> +
> >>> +    if (nb_pkts == 0) {
> >>> +        return 0;
> >>> +    }
> >>> +
> >>> +    if (nb_pkts > data->max_burst_size) {
> >>> +        VLOG_ERR("Per-flow batch size, %d, exceeds maximum limit\n",
> >> nb_pkts);
> >>> +        return 0;
> >>> +    }
> >>> +
> >>> +    for (i = 0; i < nb_pkts; i++) {
> >>> +        if (data->custom_scan(pkts[i], user_params)) {
> >>> +            pkt_buf[match_count] = pkts[i];
> >>> +            pkt_buf[match_count]->ol_flags |= PKT_TX_VLAN_PKT;
> >>
> >> Does it work if the packet already has a VLAN inserted?
> >>
> >>> Good catch. The design is based upon no VLAN insertion offloading is
> applied on source traffic.
> 
> That is a significant limitation, I haven not seen it documented in the
> series. Also, the IEEE paper mentions using QinQ when the packet has
> already a VLAN inserted, but it is not in the series. Is there a reason
> QinQ is not possible?
> 
> IIUC, that means the TAP VM would see traffic having no VLAN, whereas it
> has in reality?
> 

It does support 802.1q (single VLAN). I mis-understood your last email.
This design assumes there is no PKT_TX_VLAN_PKT flag set on the source mbuf.
This assumption is based upon that VIRTIO spec. doesn't support
per-packet VLAN insertion offloading.

> >>> +            pkt_buf[match_count]->vlan_tci = dst_vlan_id;
> >>> +            rte_mbuf_refcnt_update(pkt_buf[match_count], 1);
> >>
> >>
> >>
> >>> +            match_count++;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    dst_qidx = (data->n_dst_queue > qidx)?qidx:(data->n_dst_queue -
> 1);
> >>
> >> Wouldn't it scale better with:
> >> dst_qidx = qidx % data->n_dst_queue
> >> ?
> >>
> >>> We tried to avoid using "%" operator. We could add "unlikely" and the
> suggested "%" to make improvement
> 
> Not sure adding 'unlikely' is really necessary. The cost of the modulo
> operation is nothing compared to all we do in this path.
> 
> >>>
> >>> +
> >>> +    rte_spinlock_lock(&data->locks[dst_qidx]);
> >>> +    pkt_trans = rte_eth_tx_burst(dst_port_id, dst_qidx, pkt_buf,
> >> match_count);
> >>> +    rte_spinlock_unlock(&data->locks[dst_qidx]);
> >>> +
> >>> +    for (i = 0; i < match_count; i++) {
> >>> +        pkt_buf[i]->ol_flags &= ~PKT_TX_VLAN_PKT;
> >>> +    }
> >>
> >> In order to further reduce the performance impact of mirroring, have you
> >> envisaged to offload it to dedicated PMD threads?
> >>
> >>> The mirror-tunnel design is a comprised approach between hardware
> TAP and software TAP.
> >>> The tunnel itself is designed to have very little impact on source traffic
> processing core. From
> >>> our benchmark on SR-IOV, L2 forwarding, mirroring, we only observed
> 10-20% impact on 64-byte packets, and
> >>> we did not observe impact when running traffic with packet size with
> 128-byte or above.
> 
> The cover letter mentions 20-30% for 64B packets, and the IEEE paper
> seems to indicate a significant impact up to 512B packets (~25%?).
> 
> Maybe the workload is different in these tests, that could explain why
> you don't see an impact starting 128B?
> 

I believe the 20-30% drop is for VIRTIO port mirroring not SR-IOV mirroring.
The 512B packet (~25%?) is for default VIRTIO mirroring (OVS default
Implementation) not this design.

> >>>
> >>> +
> >>> +    while (unlikely (pkt_trans < match_count)) {
> >>> +        rte_pktmbuf_free(pkt_buf[pkt_trans]);
> >>> +        pkt_trans++;
> >>> +    }
> >>> +
> >>> +    return nb_pkts;
> >>> +}
> >>> +
> 
> ...
> 
> >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> >>> index 4597a215d..fd2049a7f 100644
> >>> --- a/vswitchd/vswitch.xml
> >>> +++ b/vswitchd/vswitch.xml
> >>> @@ -4869,11 +4869,35 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> >> type=patch options:peer=p1 \
> >>>          selected VLANs.
> >>>        </p>
> >>>
> >>> +      <column name="mirror_tunnel_addr">
> >>> +        BDF string of the tunnel device on which mirrored traffic will be
> >>> +        transmitted.
> >>> +      </column>
> >>> +
> >>>        <column name="select_all">
> >>>          If true, every packet arriving or departing on any port is
> >>>          selected for mirroring.
> >>>        </column>
> >>>
> >>> +      <column name="mirror_offload">
> >>> +        If true, a hw-assisted port mirroring is configured instead
> >>> +        default mirroring.
> >>> +      </column>
> >>> +
> >>> +      <column name="flow_src_mac">
> >>> +        The source MAC address(es) for per-flow mirroring. Each MAC
> >>> +        address is separate by ','. This parametr is paired with
> >>> +        select_dst_port. A '0' MAC address indicates the requested mirror
> >>> +        is a per-port mirroring, otherwise it's a per-flow mirroring
> >>> +      </column>
> >>> +
> >>> +      <column name="flow_dst_mac">
> >>> +        The destination MAC address(es) for per-flow mirroring. Each MAC
> >>> +        address is separate by ','. This parametr is paired with
> >>> +        select_src_port. A '0' MAC address indicates the requested mirror
> >>> +        is a per-port mirroring, otherwise it's a per-flow mirroring
> >>> +      </column>
> >>> +
> >>>        <column name="select_dst_port">
> >>>          Ports on which departing packets are selected for mirroring.
> >>>        </column>
> >>> @@ -4955,6 +4979,32 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> >> type=patch options:peer=p1 \
> >>>          </p>
> >>>        </column>
> >>>
> >>> +      <column name="output_src_vlan">
> >>> +        <p>Output VLAN for selected source port packets, if
> nonempty.</p>
> >>> +        <p>
> >>> +          <em>Please note:</em> This is different than
> >>> +          <ref column="output-vlan"/> This vlan is used to add an additional
> >>> +          vlan tag on the mirror traffic, regardless it contains vlan or not.
> >>> +          The receive end could choose to filter out this additional vlan.
> >>> +          This option is provided so the mirrored traffic could maintain its
> >>> +          original vlan informaiton, and this mirror can be used to filter
> >>> +          out un-wanted traffic such as in <ref column="mirror_offload"/>.
> >>> +        </p>
> >>> +      </column>
> >>> +
> >>> +      <column name="output_dst_vlan">
> >>> +        <p>Output VLAN for selected destination port packets, if
> >> nonempty.</p>
> >>> +        <p>
> >>> +          <em>Please note:</em> This is different than
> >>> +          <ref column="output-vlan"/> This vlan is used to add an additional
> >>> +          vlan tag on the mirror traffic, regardless it contains vlan or not.
> >>> +          The receive end could choose to filter out this additional vlan.
> >>> +          This option is provided so the mirrored traffic could maintain its
> >>> +          original vlan informaiton, and this mirror cab be used to filter
> >>> +          out un-wanted traffic such as in <ref column="mirror_offload"/>.
> >>> +        </p>
> >>> +      </column>
> >>> +
> >>>        <column name="snaplen">
> >>>          <p>Maximum per-packet number of bytes to mirror.</p>
> >>>          <p>A mirrored packet with size larger than <ref
> column="snaplen"/>
> >>>
> >
> 
> These parameters are DPDK specific, but nothing mentions it. It would
> confuse the OVS-Kernel users.
> 

The current implementation only supports OVS-DPDK.

> Regards,
> Maxime



More information about the dev mailing list