[PATCH v3] app/testpmd: expand noisy neighbour forward mode support
Kevin Traynor
ktraynor at redhat.com
Thu Jun 1 18:23:22 CEST 2023
On 26/05/2023 18:32, Mike Pattrick wrote:
> On Fri, May 26, 2023 at 11:34 AM Kevin Traynor <ktraynor at redhat.com> wrote:
>>
>> On 17/04/2023 19:55, Mike Pattrick wrote:
>>> Previously the noisy neighbour vnf simulation would only operate in io
>>> mode, forwarding packets as is. However, this limited the usefulness of
>>> noisy neighbour simulation.
>>>
>>> This feature has now been expanded to supporting mac, macswap, and
>>> 5tswap modes. To facilitate adding this support, some new header files
>>> were added.
>>>
>>
>> Hi Mike,
>>
>> I think it makes sense to allow these functionalities to be combined. It
>> does seem like that noisy neighbour shouldn't have been added as a
>> standalone fowarding option in the first place, as it's not mutually
>> exclusive with the other fwding modes. Now it's forcing a user to set a
>> fwding mode within a fwding mode to combine the functionality :/
>>
>> The approach is to allow the fwding modes to expose their forwarding
>> method so noisy can call them. Did you consider to expose the noisy
>> functionality and allow the other forwarding modes to call that when the
>> --noisy-* flags are set?
>
> Personally I like this approach more, and that was the strategy I took
> with the previous version. However, that change didn't seem popular at
> the time.
>
> http://patches.dpdk.org/project/dpdk/patch/20230126045516.176917-1-mkp@redhat.com/#157095
>
Ok, interesting, I hadn't realised that. It doesn't apply anymore since
David has restructured the fowarding engines, but i applied to older
code and took a look.
I had been thinking to an expose a function to do the noisy processing
(if any) like a do_noise() and then to return the packets to the
original fowarding engine for tx, similar to what is done the opposite
way around in v3. I can understand Aman's concerns about tx of pkts
moved from the fwding engines to noisy fwd engine.
I think we can summarize like this (will use macswp as example)
v3
- fwding engines expose their processing (e.g. do_macswp)
- user enables by setting --fwd=noisy and --noisy-fwd-mode=macswp
- noisy vnf does rx and tx
- noisy calls other fwding engine for other mode processing
- backwards compatible as existing noisy params have no effect when
fwd!=noisy
v2
- noisy engine exposes a function for noise+tx
(e.g. noisy_eth_tx_burst())
- other fwd engines call this to add noise and tx
- user enables by setting --fwd=macswp and setting existing noisy params
- breaks backwards compatibility as previously noisy params would have
no effect in macswp mode
It seems the ideal would be to have taken noisy as a feature, not a
fwding engine but it's easy in hindsight. I'm not sure it's worth
another big rework to try and add a do_noise() which may be a nice
design but will break backwards compatability. So I'd probably vote for
v3 design over v2.
What do others think ?
>> For example, a user sets fwding mode as normal 'fwd=macfwd' but then if
>> the user adds the --noisy-* params, the macfwd'ing mode calls some
>> functions exposed to add the noise, which seems a bit more intuitive.
>>
>> Also, if someone adds a new fwding mode, they can just call the noisy
>> functions and don't have update noisy fwding mode to add the new mode.
>>
>> I'm not sure if it's better, just throwing it out there as an
>> alternative idea.
>>
>> On the downside, it would break backwards compatibility because
>> previously those --noisy-* params would have had no effect with say
>> macfwd mode, but now they will. So maybe that's enough to prohibit it.
>>
>> In the past, I would have had all the params set and just changed
>> fwdmode to enable/disable noisy vnf. That would behaviour would now be
>> changed with this approach.
>>
>> What do you think?
>>
>> Few comments on the code below.
>>
>> thanks,
>> Kevin.
>>
>>> Signed-off-by: Mike Pattrick <mkp at redhat.com>
>>> ---
>>> v2: Reverted changes to random memory lookup
>>> v3: Refactored entire patch
>>> ---
>>> app/test-pmd/5tswap.c | 118 +----------------------
>>> app/test-pmd/5tswap.h | 130 ++++++++++++++++++++++++++
>>> app/test-pmd/macfwd.c | 33 +------
>>> app/test-pmd/macfwd.h | 46 +++++++++
>>> app/test-pmd/noisy_vnf.c | 92 +++++++++++++++---
>>> app/test-pmd/parameters.c | 17 ++++
>>> app/test-pmd/testpmd.c | 5 +
>>> app/test-pmd/testpmd.h | 8 ++
>>> doc/guides/testpmd_app_ug/run_app.rst | 9 ++
>>> 9 files changed, 299 insertions(+), 159 deletions(-)
>>> create mode 100644 app/test-pmd/5tswap.h
>>> create mode 100644 app/test-pmd/macfwd.h
>>>
>>> diff --git a/app/test-pmd/5tswap.c b/app/test-pmd/5tswap.c
>>> index ff8c2dcde5..8e8de2557a 100644
>>> --- a/app/test-pmd/5tswap.c
>>> +++ b/app/test-pmd/5tswap.c
>>> @@ -17,64 +17,8 @@
>>> #include <rte_ip.h>
>>> #include <rte_flow.h>
>>>
>>> -#include "macswap_common.h"
>>> #include "testpmd.h"
>>> -
>>> -
>>> -static inline void
>>> -swap_mac(struct rte_ether_hdr *eth_hdr)
>>> -{
>>> - struct rte_ether_addr addr;
>>> -
>>> - /* Swap dest and src mac addresses. */
>>> - rte_ether_addr_copy(ð_hdr->dst_addr, &addr);
>>> - rte_ether_addr_copy(ð_hdr->src_addr, ð_hdr->dst_addr);
>>> - rte_ether_addr_copy(&addr, ð_hdr->src_addr);
>>> -}
>>> -
>>> -static inline void
>>> -swap_ipv4(struct rte_ipv4_hdr *ipv4_hdr)
>>> -{
>>> - rte_be32_t addr;
>>> -
>>> - /* Swap dest and src ipv4 addresses. */
>>> - addr = ipv4_hdr->src_addr;
>>> - ipv4_hdr->src_addr = ipv4_hdr->dst_addr;
>>> - ipv4_hdr->dst_addr = addr;
>>> -}
>>> -
>>> -static inline void
>>> -swap_ipv6(struct rte_ipv6_hdr *ipv6_hdr)
>>> -{
>>> - uint8_t addr[16];
>>> -
>>> - /* Swap dest and src ipv6 addresses. */
>>> - memcpy(&addr, &ipv6_hdr->src_addr, 16);
>>> - memcpy(&ipv6_hdr->src_addr, &ipv6_hdr->dst_addr, 16);
>>> - memcpy(&ipv6_hdr->dst_addr, &addr, 16);
>>> -}
>>> -
>>> -static inline void
>>> -swap_tcp(struct rte_tcp_hdr *tcp_hdr)
>>> -{
>>> - rte_be16_t port;
>>> -
>>> - /* Swap dest and src tcp port. */
>>> - port = tcp_hdr->src_port;
>>> - tcp_hdr->src_port = tcp_hdr->dst_port;
>>> - tcp_hdr->dst_port = port;
>>> -}
>>> -
>>> -static inline void
>>> -swap_udp(struct rte_udp_hdr *udp_hdr)
>>> -{
>>> - rte_be16_t port;
>>> -
>>> - /* Swap dest and src udp port */
>>> - port = udp_hdr->src_port;
>>> - udp_hdr->src_port = udp_hdr->dst_port;
>>> - udp_hdr->dst_port = port;
>>> -}
>>> +#include "5tswap.h"
>>>
>>> /*
>>> * 5 tuple swap forwarding mode: Swap the source and the destination of layers
>>> @@ -85,22 +29,7 @@ static bool
>>> pkt_burst_5tuple_swap(struct fwd_stream *fs)
>>> {
>>> struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>>> - struct rte_port *txp;
>>> - struct rte_mbuf *mb;
>>> - uint16_t next_proto;
>>> - uint64_t ol_flags;
>>> - uint16_t proto;
>>> uint16_t nb_rx;
>>> - int i;
>>> - union {
>>> - struct rte_ether_hdr *eth;
>>> - struct rte_vlan_hdr *vlan;
>>> - struct rte_ipv4_hdr *ipv4;
>>> - struct rte_ipv6_hdr *ipv6;
>>> - struct rte_tcp_hdr *tcp;
>>> - struct rte_udp_hdr *udp;
>>> - uint8_t *byte;
>>> - } h;
>>>
>>> /*
>>> * Receive a burst of packets and forward them.
>>> @@ -109,49 +38,8 @@ pkt_burst_5tuple_swap(struct fwd_stream *fs)
>>> if (unlikely(nb_rx == 0))
>>> return false;
>>>
>>> - txp = &ports[fs->tx_port];
>>> - ol_flags = ol_flags_init(txp->dev_conf.txmode.offloads);
>>> - vlan_qinq_set(pkts_burst, nb_rx, ol_flags,
>>> - txp->tx_vlan_id, txp->tx_vlan_id_outer);
>>> - for (i = 0; i < nb_rx; i++) {
>>> - if (likely(i < nb_rx - 1))
>>> - rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i+1],
>>> - void *));
>>> - mb = pkts_burst[i];
>>> - h.eth = rte_pktmbuf_mtod(mb, struct rte_ether_hdr *);
>>> - proto = h.eth->ether_type;
>>> - swap_mac(h.eth);
>>> - mb->l2_len = sizeof(struct rte_ether_hdr);
>>> - h.eth++;
>>> - while (proto == RTE_BE16(RTE_ETHER_TYPE_VLAN) ||
>>> - proto == RTE_BE16(RTE_ETHER_TYPE_QINQ)) {
>>> - proto = h.vlan->eth_proto;
>>> - h.vlan++;
>>> - mb->l2_len += sizeof(struct rte_vlan_hdr);
>>> - }
>>> - if (proto == RTE_BE16(RTE_ETHER_TYPE_IPV4)) {
>>> - swap_ipv4(h.ipv4);
>>> - next_proto = h.ipv4->next_proto_id;
>>> - mb->l3_len = rte_ipv4_hdr_len(h.ipv4);
>>> - h.byte += mb->l3_len;
>>> - } else if (proto == RTE_BE16(RTE_ETHER_TYPE_IPV6)) {
>>> - swap_ipv6(h.ipv6);
>>> - next_proto = h.ipv6->proto;
>>> - h.ipv6++;
>>> - mb->l3_len = sizeof(struct rte_ipv6_hdr);
>>> - } else {
>>> - mbuf_field_set(mb, ol_flags);
>>> - continue;
>>> - }
>>> - if (next_proto == IPPROTO_UDP) {
>>> - swap_udp(h.udp);
>>> - mb->l4_len = sizeof(struct rte_udp_hdr);
>>> - } else if (next_proto == IPPROTO_TCP) {
>>> - swap_tcp(h.tcp);
>>> - mb->l4_len = (h.tcp->data_off & 0xf0) >> 2;
>>> - }
>>> - mbuf_field_set(mb, ol_flags);
>>> - }
>>> + do_5tswap(pkts_burst, nb_rx, fs);
>>> +
>>
>> this simplification is nice.
>>
>>> common_fwd_stream_transmit(fs, pkts_burst, nb_rx);
>>>
>>> return true;
>>> diff --git a/app/test-pmd/5tswap.h b/app/test-pmd/5tswap.h
>>> new file mode 100644
>>> index 0000000000..48da9236dc
>>> --- /dev/null
>>> +++ b/app/test-pmd/5tswap.h
>>> @@ -0,0 +1,130 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2018 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _5TSWAP_H_
>>> +#define _5TSWAP_H_
>>> +
>>> +#include "macswap_common.h"
>>> +
>>> +static inline void
>>> +swap_mac(struct rte_ether_hdr *eth_hdr)
>>> +{
>>> + struct rte_ether_addr addr;
>>> +
>>> + /* Swap dest and src mac addresses. */
>>> + rte_ether_addr_copy(ð_hdr->dst_addr, &addr);
>>> + rte_ether_addr_copy(ð_hdr->src_addr, ð_hdr->dst_addr);
>>> + rte_ether_addr_copy(&addr, ð_hdr->src_addr);
>>> +}
>>> +
>>> +static inline void
>>> +swap_ipv4(struct rte_ipv4_hdr *ipv4_hdr)
>>> +{
>>> + rte_be32_t addr;
>>> +
>>> + /* Swap dest and src ipv4 addresses. */
>>> + addr = ipv4_hdr->src_addr;
>>> + ipv4_hdr->src_addr = ipv4_hdr->dst_addr;
>>> + ipv4_hdr->dst_addr = addr;
>>> +}
>>> +
>>> +static inline void
>>> +swap_ipv6(struct rte_ipv6_hdr *ipv6_hdr)
>>> +{
>>> + uint8_t addr[16];
>>> +
>>> + /* Swap dest and src ipv6 addresses. */
>>> + memcpy(&addr, &ipv6_hdr->src_addr, 16);
>>> + memcpy(&ipv6_hdr->src_addr, &ipv6_hdr->dst_addr, 16);
>>> + memcpy(&ipv6_hdr->dst_addr, &addr, 16);
>>> +}
>>> +
>>> +static inline void
>>> +swap_tcp(struct rte_tcp_hdr *tcp_hdr)
>>> +{
>>> + rte_be16_t port;
>>> +
>>> + /* Swap dest and src tcp port. */
>>> + port = tcp_hdr->src_port;
>>> + tcp_hdr->src_port = tcp_hdr->dst_port;
>>> + tcp_hdr->dst_port = port;
>>> +}
>>> +
>>> +static inline void
>>> +swap_udp(struct rte_udp_hdr *udp_hdr)
>>> +{
>>> + rte_be16_t port;
>>> +
>>> + /* Swap dest and src udp port */
>>> + port = udp_hdr->src_port;
>>> + udp_hdr->src_port = udp_hdr->dst_port;
>>> + udp_hdr->dst_port = port;
>>> +}
>>> +
>>> +static inline void
>>> +do_5tswap(struct rte_mbuf *pkts_burst[], uint16_t nb_rx,
>>> + struct fwd_stream *fs)
>>> +{
>>> + struct rte_port *txp;
>>> + struct rte_mbuf *mb;
>>> + uint16_t next_proto;
>>> + uint64_t ol_flags;
>>> + uint16_t proto;
>>> + int i;
>>> + union {
>>> + struct rte_ether_hdr *eth;
>>> + struct rte_vlan_hdr *vlan;
>>> + struct rte_ipv4_hdr *ipv4;
>>> + struct rte_ipv6_hdr *ipv6;
>>> + struct rte_tcp_hdr *tcp;
>>> + struct rte_udp_hdr *udp;
>>> + uint8_t *byte;
>>> + } h;
>>> +
>>> + txp = &ports[fs->tx_port];
>>> + ol_flags = ol_flags_init(txp->dev_conf.txmode.offloads);
>>> + vlan_qinq_set(pkts_burst, nb_rx, ol_flags,
>>> + txp->tx_vlan_id, txp->tx_vlan_id_outer);
>>> + for (i = 0; i < nb_rx; i++) {
>>> + if (likely(i < nb_rx - 1))
>>> + rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i+1],
>>> + void *));
>>> + mb = pkts_burst[i];
>>> + h.eth = rte_pktmbuf_mtod(mb, struct rte_ether_hdr *);
>>> + proto = h.eth->ether_type;
>>> + swap_mac(h.eth);
>>> + mb->l2_len = sizeof(struct rte_ether_hdr);
>>> + h.eth++;
>>> + while (proto == RTE_BE16(RTE_ETHER_TYPE_VLAN) ||
>>> + proto == RTE_BE16(RTE_ETHER_TYPE_QINQ)) {
>>> + proto = h.vlan->eth_proto;
>>> + h.vlan++;
>>> + mb->l2_len += sizeof(struct rte_vlan_hdr);
>>> + }
>>> + if (proto == RTE_BE16(RTE_ETHER_TYPE_IPV4)) {
>>> + swap_ipv4(h.ipv4);
>>> + next_proto = h.ipv4->next_proto_id;
>>> + mb->l3_len = rte_ipv4_hdr_len(h.ipv4);
>>> + h.byte += mb->l3_len;
>>> + } else if (proto == RTE_BE16(RTE_ETHER_TYPE_IPV6)) {
>>> + swap_ipv6(h.ipv6);
>>> + next_proto = h.ipv6->proto;
>>> + h.ipv6++;
>>> + mb->l3_len = sizeof(struct rte_ipv6_hdr);
>>> + } else {
>>> + mbuf_field_set(mb, ol_flags);
>>> + continue;
>>> + }
>>> + if (next_proto == IPPROTO_UDP) {
>>> + swap_udp(h.udp);
>>> + mb->l4_len = sizeof(struct rte_udp_hdr);
>>> + } else if (next_proto == IPPROTO_TCP) {
>>> + swap_tcp(h.tcp);
>>> + mb->l4_len = (h.tcp->data_off & 0xf0) >> 2;
>>> + }
>>> + mbuf_field_set(mb, ol_flags);
>>> + }
>>> +}
>>> +
>>> +#endif /* _5TSWAP_H_ */
>>> diff --git a/app/test-pmd/macfwd.c b/app/test-pmd/macfwd.c
>>> index 7316d73315..d19ace7395 100644
>>> --- a/app/test-pmd/macfwd.c
>>> +++ b/app/test-pmd/macfwd.c
>>> @@ -35,6 +35,7 @@
>>> #include <rte_flow.h>
>>>
>>> #include "testpmd.h"
>>> +#include "macfwd.h"
>>>
>>> /*
>>> * Forwarding of packets in MAC mode.
>>> @@ -45,13 +46,7 @@ static bool
>>> pkt_burst_mac_forward(struct fwd_stream *fs)
>>> {
>>> struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>>> - struct rte_port *txp;
>>> - struct rte_mbuf *mb;
>>> - struct rte_ether_hdr *eth_hdr;
>>> uint16_t nb_rx;
>>> - uint16_t i;
>>> - uint64_t ol_flags = 0;
>>> - uint64_t tx_offloads;
>>>
>>> /*
>>> * Receive a burst of packets and forward them.
>>> @@ -60,31 +55,7 @@ pkt_burst_mac_forward(struct fwd_stream *fs)
>>> if (unlikely(nb_rx == 0))
>>> return false;
>>>
>>> - txp = &ports[fs->tx_port];
>>> - tx_offloads = txp->dev_conf.txmode.offloads;
>>> - if (tx_offloads & RTE_ETH_TX_OFFLOAD_VLAN_INSERT)
>>> - ol_flags = RTE_MBUF_F_TX_VLAN;
>>> - if (tx_offloads & RTE_ETH_TX_OFFLOAD_QINQ_INSERT)
>>> - ol_flags |= RTE_MBUF_F_TX_QINQ;
>>> - if (tx_offloads & RTE_ETH_TX_OFFLOAD_MACSEC_INSERT)
>>> - ol_flags |= RTE_MBUF_F_TX_MACSEC;
>>> - for (i = 0; i < nb_rx; i++) {
>>> - if (likely(i < nb_rx - 1))
>>> - rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
>>> - void *));
>>> - mb = pkts_burst[i];
>>> - eth_hdr = rte_pktmbuf_mtod(mb, struct rte_ether_hdr *);
>>> - rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
>>> - ð_hdr->dst_addr);
>>> - rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
>>> - ð_hdr->src_addr);
>>> - mb->ol_flags &= RTE_MBUF_F_INDIRECT | RTE_MBUF_F_EXTERNAL;
>>> - mb->ol_flags |= ol_flags;
>>> - mb->l2_len = sizeof(struct rte_ether_hdr);
>>> - mb->l3_len = sizeof(struct rte_ipv4_hdr);
>>> - mb->vlan_tci = txp->tx_vlan_id;
>>> - mb->vlan_tci_outer = txp->tx_vlan_id_outer;
>>> - }
>>> + do_macfwd(pkts_burst, nb_rx, fs);
>>>
>>> common_fwd_stream_transmit(fs, pkts_burst, nb_rx);
>>>
>>> diff --git a/app/test-pmd/macfwd.h b/app/test-pmd/macfwd.h
>>> new file mode 100644
>>> index 0000000000..3f3e7189e1
>>> --- /dev/null
>>> +++ b/app/test-pmd/macfwd.h
>>> @@ -0,0 +1,46 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2018 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _MACFWD_H_
>>> +#define _MACFWD_H_
>>> +
>>> +static inline void
>>> +do_macfwd(struct rte_mbuf *pkts_burst[], uint16_t nb_rx,
>>> + struct fwd_stream *fs)
>>
>> nit: indent/alignment is a little off here. There is some extra spaces.
>> Same with other do_*()
>>
>>> +{
>>> + struct rte_ether_hdr *eth_hdr;
>>> + uint64_t ol_flags = 0;
>>> + uint64_t tx_offloads;
>>> + struct rte_mbuf *mb;
>>> + struct rte_port *txp = &ports[fs->tx_port];
>>> + uint16_t i;
>>> +
>>> +
>>
>> can remove addition blank line
>>
>>> + tx_offloads = txp->dev_conf.txmode.offloads;
>>> + if (tx_offloads & RTE_ETH_TX_OFFLOAD_VLAN_INSERT)
>>> + ol_flags = RTE_MBUF_F_TX_VLAN;
>>> + if (tx_offloads & RTE_ETH_TX_OFFLOAD_QINQ_INSERT)
>>> + ol_flags |= RTE_MBUF_F_TX_QINQ;
>>> + if (tx_offloads & RTE_ETH_TX_OFFLOAD_MACSEC_INSERT)
>>> + ol_flags |= RTE_MBUF_F_TX_MACSEC;
>>> + for (i = 0; i < nb_rx; i++) {
>>> + if (likely(i < nb_rx - 1))
>>> + rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
>>> + void *));
>>> + mb = pkts_burst[i];
>>> + eth_hdr = rte_pktmbuf_mtod(mb, struct rte_ether_hdr *);
>>> + rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
>>> + ð_hdr->dst_addr);
>>> + rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
>>> + ð_hdr->src_addr);
>>> + mb->ol_flags &= RTE_MBUF_F_INDIRECT | RTE_MBUF_F_EXTERNAL;
>>> + mb->ol_flags |= ol_flags;
>>> + mb->l2_len = sizeof(struct rte_ether_hdr);
>>> + mb->l3_len = sizeof(struct rte_ipv4_hdr);
>>> + mb->vlan_tci = txp->tx_vlan_id;
>>> + mb->vlan_tci_outer = txp->tx_vlan_id_outer;
>>> + }
>>> +}
>>> +
>>> +#endif /* _MACFWD_H_ */
>>> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
>>> index 2bf90a983c..1d5a2e470a 100644
>>> --- a/app/test-pmd/noisy_vnf.c
>>> +++ b/app/test-pmd/noisy_vnf.c
>>> @@ -32,6 +32,18 @@
>>> #include <rte_malloc.h>
>>>
>>> #include "testpmd.h"
>>> +#include "5tswap.h"
>>> +#include "macfwd.h"
>>> +#if defined(RTE_ARCH_X86)
>>> +#include "macswap_sse.h"
>>> +#elif defined(__ARM_NEON)
>>> +#include "macswap_neon.h"
>>> +#else
>>> +#include "macswap.h"
>>> +#endif
>>> +
>>> +#define NOISY_STRSIZE 256
>>> +#define NOISY_RING "noisy_ring_%d\n"
>>>
>>> struct noisy_config {
>>> struct rte_ring *f;
>>> @@ -80,9 +92,6 @@ sim_memory_lookups(struct noisy_config *ncf, uint16_t nb_pkts)
>>> {
>>> uint16_t i, j;
>>>
>>> - if (!ncf->do_sim)
>>> - return;
>>> -
>>
>> Why is this removed? It's not checked before all calls to this function
>>
>>> for (i = 0; i < nb_pkts; i++) {
>>> for (j = 0; j < noisy_lkup_num_writes; j++)
>>> do_write(ncf->vnf_mem);
>>> @@ -110,15 +119,13 @@ sim_memory_lookups(struct noisy_config *ncf, uint16_t nb_pkts)
>>> * out of the FIFO
>>> * 4. Cases 2 and 3 combined
>>> */
>>> -static bool
>>> -pkt_burst_noisy_vnf(struct fwd_stream *fs)
>>> +static uint16_t
>>> +noisy_eth_tx_burst(struct fwd_stream *fs, uint16_t nb_rx, struct rte_mbuf **pkts_burst)
>>> {
>>> const uint64_t freq_khz = rte_get_timer_hz() / 1000;
>>> struct noisy_config *ncf = noisy_cfg[fs->rx_port];
>>> - struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>>> struct rte_mbuf *tmp_pkts[MAX_PKT_BURST];
>>> uint16_t nb_deqd = 0;
>>> - uint16_t nb_rx = 0;
>>> uint16_t nb_tx = 0;
>>> uint16_t nb_enqd;
>>> unsigned int fifo_free;
>>> @@ -126,12 +133,12 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>>> bool needs_flush = false;
>>> uint64_t now;
>>>
>>> - nb_rx = common_fwd_stream_receive(fs, pkts_burst, nb_pkt_per_burst);
>>> if (unlikely(nb_rx == 0))
>>> goto flush;
>>>
>>> if (!ncf->do_buffering) {
>>> - sim_memory_lookups(ncf, nb_rx);
>>> + if (ncf->do_sim)
>>> + sim_memory_lookups(ncf, nb_rx);
>>> nb_tx = common_fwd_stream_transmit(fs, pkts_burst, nb_rx);
>>> goto end;
>>> }
>>> @@ -169,11 +176,61 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>>> ncf->prev_time = rte_get_timer_cycles();
>>> }
>>> end:
>>> + return nb_tx;
>>> +}
>>> +
>>> +static bool
>>> +pkt_burst_io(struct fwd_stream *fs)
>>> +{
>>> + struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>>> + uint16_t nb_rx;
>>> + uint16_t nb_tx;
>>> +
>>> + nb_rx = common_fwd_stream_receive(fs, pkts_burst, nb_pkt_per_burst);
>>> + nb_tx = noisy_eth_tx_burst(fs, nb_rx, pkts_burst);
>>> +
>>> return nb_rx > 0 || nb_tx > 0;
>>> }
>>>
>>> -#define NOISY_STRSIZE 256
>>> -#define NOISY_RING "noisy_ring_%d\n"
>>> +static bool
>>> +pkt_burst_mac(struct fwd_stream *fs)
>>> +{
>>> + struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>>> + uint16_t nb_rx;
>>> + uint16_t nb_tx;
>>> +
>>> + nb_rx = common_fwd_stream_receive(fs, pkts_burst, nb_pkt_per_burst);
>>
>> Similar to macfwd.c, you can add check for unlikely(nb_rx == 0) at this
>> point in these fns.
>
> That's true. I removed it because I'll have to call
> noisy_eth_tx_burst() regardless, but we could avoid the middle call in
> these locations.
>
> I agree with the other suggestions.
>
> Thanks,
> M
>
>>
>>> + do_macfwd(pkts_burst, nb_rx, fs);
>>> + nb_tx = noisy_eth_tx_burst(fs, nb_rx, pkts_burst);
>>> +
>>> + return nb_rx > 0 || nb_tx > 0;
>>> +}
>>> +static bool
>>> +pkt_burst_macswap(struct fwd_stream *fs)
>>> +{
>>> + struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>>> + uint16_t nb_rx = 0;
>>> + uint16_t nb_tx = 0;
>>
>> nit: these are not assigned in the other functions
>>
>>> +
>>> + nb_rx = common_fwd_stream_receive(fs, pkts_burst, nb_pkt_per_burst);
>>> + do_macswap(pkts_burst, nb_rx, &ports[fs->tx_port]);
>>> + nb_tx = noisy_eth_tx_burst(fs, nb_rx, pkts_burst);
>>> +
>>> + return nb_rx > 0 || nb_tx > 0;
>>> +}
>>> +static bool
>>> +pkt_brust_5tswap(struct fwd_stream *fs)
>>> +{
>>> + struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>>> + uint16_t nb_rx = 0;
>>> + uint16_t nb_tx = 0;
>>> +
>>> + nb_rx = common_fwd_stream_receive(fs, pkts_burst, nb_pkt_per_burst);
>>> + do_5tswap(pkts_burst, nb_rx, fs);
>>> + nb_tx = noisy_eth_tx_burst(fs, nb_rx, pkts_burst);
>>> +
>>> + return nb_rx > 0 || nb_tx > 0;
>>> +}
>>>
>>> static void
>>> noisy_fwd_end(portid_t pi)
>>> @@ -226,6 +283,15 @@ noisy_fwd_begin(portid_t pi)
>>> "--noisy-lkup-memory-size must be > 0\n");
>>> }
>>>
>>> + if (noisy_fwd_mode == NOISY_FWD_MODE_IO)
>>> + noisy_vnf_engine.packet_fwd = pkt_burst_io;
>>> + else if (noisy_fwd_mode == NOISY_FWD_MODE_MAC)
>>> + noisy_vnf_engine.packet_fwd = pkt_burst_mac;
>>> + else if (noisy_fwd_mode == NOISY_FWD_MODE_MACSWAP)
>>> + noisy_vnf_engine.packet_fwd = pkt_burst_macswap;
>>> + else if (noisy_fwd_mode == NOISY_FWD_MODE_5TSWAP)
>>> + noisy_vnf_engine.packet_fwd = pkt_brust_5tswap;
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -233,6 +299,6 @@ struct fwd_engine noisy_vnf_engine = {
>>> .fwd_mode_name = "noisy",
>>> .port_fwd_begin = noisy_fwd_begin,
>>> .port_fwd_end = noisy_fwd_end,
>>> - .stream_init = common_fwd_stream_init,
>>> - .packet_fwd = pkt_burst_noisy_vnf,
>>> + .stream_init = common_fwd_stream_init,
>>> + .packet_fwd = pkt_burst_io,
>>> };
>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>>> index 3b37809baf..129c55c0ad 100644
>>> --- a/app/test-pmd/parameters.c
>>> +++ b/app/test-pmd/parameters.c
>>> @@ -196,6 +196,7 @@ usage(char* progname)
>>> printf(" --noisy-lkup-num-writes=N: do N random writes per packet\n");
>>> printf(" --noisy-lkup-num-reads=N: do N random reads per packet\n");
>>> printf(" --noisy-lkup-num-reads-writes=N: do N random reads and writes per packet\n");
>>> + printf(" --noisy-fwd-mode=mode: set the fwd mode\n");
>>> printf(" --no-iova-contig: mempool memory can be IOVA non contiguous. "
>>> "valid only with --mp-alloc=anon\n");
>>> printf(" --rx-mq-mode=0xX: hexadecimal bitmask of RX mq mode can be "
>>> @@ -704,6 +705,7 @@ launch_args_parse(int argc, char** argv)
>>> { "noisy-lkup-num-writes", 1, 0, 0 },
>>> { "noisy-lkup-num-reads", 1, 0, 0 },
>>> { "noisy-lkup-num-reads-writes", 1, 0, 0 },
>>> + { "noisy-fwd-mode", 1, 0, 0 },
>>> { "no-iova-contig", 0, 0, 0 },
>>> { "rx-mq-mode", 1, 0, 0 },
>>> { "record-core-cycles", 0, 0, 0 },
>>> @@ -1444,6 +1446,21 @@ launch_args_parse(int argc, char** argv)
>>> rte_exit(EXIT_FAILURE,
>>> "noisy-lkup-num-reads-writes must be >= 0\n");
>>> }
>>> + if (!strcmp(lgopts[opt_idx].name,
>>> + "noisy-fwd-mode")) {
>>> + if (!strcmp(optarg, "io"))
>>> + noisy_fwd_mode = NOISY_FWD_MODE_IO;
>>> + else if (!strcmp(optarg, "mac"))
>>> + noisy_fwd_mode = NOISY_FWD_MODE_MAC;
>>> + else if (!strcmp(optarg, "macswap"))
>>> + noisy_fwd_mode = NOISY_FWD_MODE_MACSWAP;
>>> + else if (!strcmp(optarg, "5tswap"))
>>> + noisy_fwd_mode = NOISY_FWD_MODE_5TSWAP;
>>> + else
>>> + rte_exit(EXIT_FAILURE, "noisy-fwd-mode %s invalid -"
>>> + " must b a valid fwd mode\n",
>>
>> typo "be"
>>
>> I would specify "..must be a valid noisy-fwd-mode value" to avoid
>> confusion with the full set of general fwd modes and easier for user to
>> search them in docs.
>>
>>> + optarg);
>>> + }
>>> if (!strcmp(lgopts[opt_idx].name, "no-iova-contig"))
>>> mempool_flags = RTE_MEMPOOL_F_NO_IOVA_CONTIG;
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index 5cb6f92523..92784873ff 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -364,6 +364,11 @@ uint64_t noisy_lkup_num_reads;
>>> */
>>> uint64_t noisy_lkup_num_reads_writes;
>>>
>>> +/*
>>> + * Configurable forwarding mode in VNF simulation.
>>> + */
>>> +int noisy_fwd_mode;
>>> +
>>> /*
>>> * Receive Side Scaling (RSS) configuration.
>>> */
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>> index bdfbfd36d3..f70397ad26 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -116,6 +116,13 @@ enum {
>>> QUEUE_JOB_TYPE_ACTION_QUERY,
>>> };
>>>
>>> +enum {
>>> + NOISY_FWD_MODE_IO,
>>> + NOISY_FWD_MODE_MAC,
>>> + NOISY_FWD_MODE_MACSWAP,
>>> + NOISY_FWD_MODE_5TSWAP
>>> +};
>>> +
>>> /**
>>> * The data structure associated with RX and TX packet burst statistics
>>> * that are recorded for each forwarding stream.
>>> @@ -561,6 +568,7 @@ extern uint64_t noisy_lkup_mem_sz;
>>> extern uint64_t noisy_lkup_num_writes;
>>> extern uint64_t noisy_lkup_num_reads;
>>> extern uint64_t noisy_lkup_num_reads_writes;
>>> +extern int noisy_fwd_mode;
>>>
>>> extern uint8_t dcb_config;
>>>
>>> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
>>> index 57b23241cf..fcca3e8921 100644
>>> --- a/doc/guides/testpmd_app_ug/run_app.rst
>>> +++ b/doc/guides/testpmd_app_ug/run_app.rst
>>> @@ -519,6 +519,15 @@ The command line options are:
>>> Set the number of r/w accesses to be done in noisy neighbor simulation memory buffer to N.
>>> Only available with the noisy forwarding mode. The default value is 0.
>>>
>>> +* ``--noisy-fwd-mode=mode``
>>> +
>>> + Set the noisy vnf forwarding mode where ``mode`` is one of the following::
>>> +
>>> + io (the default)
>>> + mac
>>> + macswap
>>> + 5tswap
>>> +
>>> * ``--no-iova-contig``
>>>
>>> Enable to create mempool which is not IOVA contiguous. Valid only with --mp-alloc=anon.
>>
>
More information about the dev
mailing list