Re: [dpdk-dev] [PATCH v4] net/bonding: per-slave intermediate rx ring

[Matan Azrad <matan@mellanox.com>]

Hi Chas

From: Chas Williams
> On Tue, Aug 21, 2018 at 6:56 AM Matan Azrad <matan@mellanox.com>
> wrote:
> Hi
> 
> From: Chas Williams
> > This will need to be implemented for some of the other RX burst
> > methods at some point for other modes to see this performance
> > improvement (with the exception of active-backup).
> 
> Yes, I think it should be done at least to
> bond_ethdev_rx_burst_8023ad_fast_queue (should be easy) for now.
> 
> There is some duplicated code between the various RX paths.
> I would like to eliminate that as much as possible, so I was going to give that
> some thought first.

There is no reason to stay this function as is while its twin is changed.

> 
> 
> > On Thu, Aug 16, 2018 at 9:32 AM Luca Boccassi <bluca@debian.org> wrote:
> >
> > > During bond 802.3ad receive, a burst of packets is fetched from each
> > > slave into a local array and appended to per-slave ring buffer.
> > > Packets are taken from the head of the ring buffer and returned to
> > > the caller.  The number of mbufs provided to each slave is
> > > sufficient to meet the requirements of the ixgbe vector receive.
> 
> Luca,
> 
> Can you explain these requirements of ixgbe?
> 
> The ixgbe (and some other Intel PMDs) have vectorized RX routines that are
> more efficient (if not faster) taking advantage of some advanced CPU
> instructions.  I think you need to be receiving at least 32 packets or more.

So, why to do it in bond which is a generic driver for all the vendors PMDs,
If for ixgbe and other Intel nics it is better you can force those PMDs to receive always 32 packets
and to manage a ring by themselves.
 
> Did you check for other vendor PMDs? It may hurt performance there..
> 
> I don't know, but I suspect probably not.  For the most part you are typically
> reading almost up to the vector requirement.  But if one slave has just a
> single packet, then you can't vectorize on the next slave.
> 

I don't think that the ring overhead is better for PMDs which are not using the vectorized instructions.

> Empirically, this patch has been around of years in one form or another.  No
> complaints other than "bonding is slow" which is why it was written.
> 
> 
> > >
> > > This small change improves performances of the bonding driver
> > > considerably. Vyatta has been using it for years in production.
> > >
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Eric Kinzie <ekinzie@brocade.com>
> > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > >
> >
> > Acked-by: Chas Williams <chas3@att.com>
> >
> >
> >
> > > ---
> > > v2 and v3: fix checkpatch warnings
> > > v4: add Eric's original signed-off-by from the Vyatta internal repo
> > >
> > >  drivers/net/bonding/rte_eth_bond_api.c     | 13 +++
> > >  drivers/net/bonding/rte_eth_bond_pmd.c     | 98
> +++++++++++++++++--
> > ---
> > >  drivers/net/bonding/rte_eth_bond_private.h |  4 +
> > >  3 files changed, 95 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/net/bonding/rte_eth_bond_api.c
> > > b/drivers/net/bonding/rte_eth_bond_api.c
> > > index 8bc04cfd11..3d22203e91 100644
> > > --- a/drivers/net/bonding/rte_eth_bond_api.c
> > > +++ b/drivers/net/bonding/rte_eth_bond_api.c
> > > @@ -524,6 +524,10 @@ __eth_bond_slave_remove_lock_free(uint16_t
> > > bonded_port_id,
> > >
> > > sizeof(*(rte_eth_devices[bonded_port_id].data->mac_addrs)));
> > >         }
> > >         if (internals->slave_count == 0) {
> > > +               /* Remove any remaining packets in the receive ring */
> > > +               struct rte_mbuf *bufs[PMD_BOND_RECV_PKTS_PER_SLAVE];
> > > +               unsigned int j, count;
> > > +
> > >                 internals->rx_offload_capa = 0;
> > >                 internals->tx_offload_capa = 0;
> > >                 internals->rx_queue_offload_capa = 0; @@ -532,6
> > > +536,15 @@ __eth_bond_slave_remove_lock_free(uint16_t
> > > bonded_port_id,
> > >                 internals->reta_size = 0;
> > >                 internals->candidate_max_rx_pktlen = 0;
> > >                 internals->max_rx_pktlen = 0;
> > > +
> > > +               do {
> > > +                       count = rte_ring_dequeue_burst(internals->rx_ring,
> > > +                                       (void **)bufs,
> > > +                                       PMD_BOND_RECV_PKTS_PER_SLAVE,
> > > +                                       NULL);
> > > +                       for  (j = 0; j < count; j++)
> > > +                               rte_pktmbuf_free(bufs[j]);
> > > +               } while (count > 0);
> > >         }
> > >         return 0;
> > >  }
> > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > index 58f7377c60..ae756c4e3a 100644
> > > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > @@ -18,6 +18,8 @@
> > >  #include <rte_alarm.h>
> > >  #include <rte_cycles.h>
> > >  #include <rte_string_fns.h>
> > > +#include <rte_errno.h>
> > > +#include <rte_lcore.h>
> > >
> > >  #include "rte_eth_bond.h"
> > >  #include "rte_eth_bond_private.h"
> > > @@ -402,10 +404,15 @@ bond_ethdev_rx_burst_8023ad(void *queue,
> > struct
> > > rte_mbuf **bufs,
> > >         struct bond_dev_private *internals = bd_rx_q->dev_private;
> > >         struct ether_addr bond_mac;
> > >
> > > +       unsigned int rx_ring_avail =
> > > rte_ring_free_count(internals->rx_ring);
> > > +       struct rte_mbuf
> > *mbuf_bounce[PMD_BOND_RECV_PKTS_PER_SLAVE];
> > > +
> > >         struct ether_hdr *hdr;
> > >
> > >         const uint16_t ether_type_slow_be =
> > > rte_be_to_cpu_16(ETHER_TYPE_SLOW);
> > >         uint16_t num_rx_total = 0;      /* Total number of received
> > > packets */
> > > +       uint16_t num_rx_slave;
> > > +       uint16_t num_enq_slave;
> > >         uint16_t slaves[RTE_MAX_ETHPORTS];
> > >         uint16_t slave_count, idx;
> > >
> > > @@ -414,6 +421,9 @@ bond_ethdev_rx_burst_8023ad(void *queue,
> > struct
> > > rte_mbuf **bufs,
> > >         uint8_t i, j, k;
> > >         uint8_t subtype;
> > >
> > > +       if (rx_ring_avail < PMD_BOND_RECV_PKTS_PER_SLAVE)
> > > +               goto dequeue;
> > > +
> > >         rte_eth_macaddr_get(internals->port_id, &bond_mac);
> > >         /* Copy slave list to protect against slave up/down changes
> > > during tx
> > >          * bursting */
> > > @@ -426,62 +436,96 @@ bond_ethdev_rx_burst_8023ad(void *queue,
> > struct
> > > rte_mbuf **bufs,
> > >                 internals->active_slave = 0;
> > >                 idx = 0;
> > >         }
> > > -       for (i = 0; i < slave_count && num_rx_total < nb_pkts; i++) {
> > > -               j = num_rx_total;
> > > +       for (i = 0; i < slave_count && num_rx_total < rx_ring_avail; i++) {
> > > +               j = 0;
> > >                 collecting = ACTOR_STATE(&mode_8023ad_ports[slaves[idx]],
> > >                                          COLLECTING);
> > >
> > >                 /* Read packets from this slave */
> > > -               num_rx_total += rte_eth_rx_burst(slaves[idx],
> > > bd_rx_q->queue_id,
> > > -                               &bufs[num_rx_total], nb_pkts -
> > > num_rx_total);
> > > +               if (unlikely(rx_ring_avail - num_rx_total <
> > > +                               PMD_BOND_RECV_PKTS_PER_SLAVE))
> > > +                       continue;
> > > +               num_rx_slave = rte_eth_rx_burst(slaves[idx],
> > > bd_rx_q->queue_id,
> > > +                               mbuf_bounce,
> > > + PMD_BOND_RECV_PKTS_PER_SLAVE);
> > >
> > > -               for (k = j; k < 2 && k < num_rx_total; k++)
> > > -                       rte_prefetch0(rte_pktmbuf_mtod(bufs[k], void *));
> > > +               for (k = j; k < 2 && k < num_rx_slave; k++)
> > > +
> > > + rte_prefetch0(rte_pktmbuf_mtod(mbuf_bounce[k],
> > > void *));
> > >
> > >                 /* Handle slow protocol packets. */
> > > -               while (j < num_rx_total) {
> > > +               while (j < num_rx_slave) {
> > >
> > >                         /* If packet is not pure L2 and is known,
> > > skip it */
> > > -                       if ((bufs[j]->packet_type & ~RTE_PTYPE_L2_ETHER)
> > > != 0) {
> > > +                       if ((mbuf_bounce[j]->packet_type &
> > > ~RTE_PTYPE_L2_ETHER) != 0) {
> > >                                 j++;
> > >                                 continue;
> > >                         }
> > >
> > > -                       if (j + 3 < num_rx_total)
> > > -                               rte_prefetch0(rte_pktmbuf_mtod(bufs[j +
> > > 3], void *));
> > > +                       if (j + 3 < num_rx_slave)
> > > +
> > >  rte_prefetch0(rte_pktmbuf_mtod(mbuf_bounce[j + 3],
> > > +                                                              void
> > > + *));
> > >
> > > -                       hdr = rte_pktmbuf_mtod(bufs[j], struct ether_hdr
> > > *);
> > > +                       hdr = rte_pktmbuf_mtod(mbuf_bounce[j],
> > > +                                              struct ether_hdr *);
> > >                         subtype = ((struct slow_protocol_frame
> > > *)hdr)->slow_protocol.subtype;
> > >
> > >                         /* Remove packet from array if it is slow
> > > packet or slave is not
> > >                          * in collecting state or bonding interface
> > > is not in promiscuous
> > >                          * mode and packet address does not match. */
> > > -                       if (unlikely(is_lacp_packets(hdr->ether_type,
> > > subtype, bufs[j]) ||
> > > +                       if (unlikely(is_lacp_packets(hdr->ether_type,
> > > +                                                    subtype,
> > > mbuf_bounce[j]) ||
> > >                                 !collecting || (!promisc &&
> > >
> > > !is_multicast_ether_addr(&hdr->d_addr) &&
> > >
> > > !is_same_ether_addr(&bond_mac,
> > > &hdr->d_addr)))) {
> > >
> > >                                 if (hdr->ether_type ==
> > > ether_type_slow_be) {
> > >                                         bond_mode_8023ad_handle_slow_pkt(
> > > -                                           internals, slaves[idx],
> > > bufs[j]);
> > > +                                           internals, slaves[idx],
> > > +                                           mbuf_bounce[j]);
> > >                                 } else
> > > -                                       rte_pktmbuf_free(bufs[j]);
> > > +
> > > + rte_pktmbuf_free(mbuf_bounce[j]);
> > >
> > >                                 /* Packet is managed by mode 4 or
> > > dropped, shift the array */
> > > -                               num_rx_total--;
> > > -                               if (j < num_rx_total) {
> > > -                                       memmove(&bufs[j], &bufs[j + 1],
> > > sizeof(bufs[0]) *
> > > -                                               (num_rx_total - j));
> > > +                               num_rx_slave--;
> > > +                               if (j < num_rx_slave) {
> > > +                                       memmove(&mbuf_bounce[j],
> > > +                                               &mbuf_bounce[j + 1],
> > > +                                               sizeof(mbuf_bounce[0]) *
> > > +                                               (num_rx_slave - j));
> > >                                 }
> > > -                       } else
> > > +                       } else {
> > >                                 j++;
> > > +                       }
> > >                 }
> > > +
> > > +               if (num_rx_slave > 0) {
> > > +                       if (mbuf_bounce[0] == NULL)
> > > +                               RTE_LOG(ERR, PMD, "%s: Enqueue a NULL??\n",
> > > +                                       __func__);
> > > +
> > > +                       num_enq_slave =
> > > rte_ring_enqueue_burst(internals->rx_ring,
> > > +                                                              (void
> > > **)mbuf_bounce,
> > > +
> > > num_rx_slave,
> > > +
> > > + NULL);
> > > +
> > > +                       if (num_enq_slave < num_rx_slave) {
> > > +                               RTE_LOG(ERR, PMD,
> > > +                                       "%s: failed to enqueue %u packets",
> > > +                                       __func__,
> > > +                                       (num_rx_slave - num_enq_slave));
> > > +                               for (j = num_enq_slave; j <
> > > + num_rx_slave;
> > > j++)
> > > +                                       rte_pktmbuf_free(mbuf_bounce[j]);
> > > +                       }
> > > +                       num_rx_total += num_enq_slave;
> > > +               }
> > > +
> > >                 if (unlikely(++idx == slave_count))
> > >                         idx = 0;
> > >         }
> > >
> > >         internals->active_slave = idx;
> > > -       return num_rx_total;
> > > +dequeue:
> > > +       return rte_ring_dequeue_burst(internals->rx_ring, (void **)bufs,
> > > +                                     nb_pkts, NULL);
> > >  }
> > >
> > >  #if defined(RTE_LIBRTE_BOND_DEBUG_ALB) ||
> > > defined(RTE_LIBRTE_BOND_DEBUG_ALB_L1)
> > > @@ -3065,6 +3109,7 @@ bond_alloc(struct rte_vdev_device *dev,
> > > uint8_t
> > mode)
> > >         struct bond_dev_private *internals = NULL;
> > >         struct rte_eth_dev *eth_dev = NULL;
> > >         uint32_t vlan_filter_bmp_size;
> > > +       char mem_name[RTE_ETH_NAME_MAX_LEN];
> > >
> > >         /* now do all data allocation - for eth_dev structure, dummy
> > > pci driver
> > >          * and internal (private) data @@ -3129,6 +3174,19 @@
> > > bond_alloc(struct rte_vdev_device *dev, uint8_t
> > > mode)
> > >         TAILQ_INIT(&internals->flow_list);
> > >         internals->flow_isolated_valid = 0;
> > >
> > > +       snprintf(mem_name, RTE_DIM(mem_name), "bond_%u_rx",
> > > internals->port_id);
> > > +       internals->rx_ring = rte_ring_lookup(mem_name);
> > > +       if (internals->rx_ring == NULL) {
> > > +               internals->rx_ring = rte_ring_create(mem_name,
> > > +
> > > rte_align32pow2(PMD_BOND_RECV_RING_PKTS *
> > > +                                                    rte_lcore_count()),
> > > +                                    socket_id, 0);
> > > +               if (internals->rx_ring == NULL)
> > > +                       rte_panic("%s: Failed to create rx ring '%s':
> > > %s\n", name,
> > > +                                 mem_name, rte_strerror(rte_errno));
> > > +       }
> > > +
> > > +
> > >         /* Set mode 4 default configuration */
> > >         bond_mode_8023ad_setup(eth_dev, NULL);
> > >         if (bond_ethdev_mode_set(eth_dev, mode)) { diff --git
> > > a/drivers/net/bonding/rte_eth_bond_private.h
> > > b/drivers/net/bonding/rte_eth_bond_private.h
> > > index 43e0e448df..80261c6b14 100644
> > > --- a/drivers/net/bonding/rte_eth_bond_private.h
> > > +++ b/drivers/net/bonding/rte_eth_bond_private.h
> > > @@ -26,6 +26,8 @@
> > >  #define PMD_BOND_LSC_POLL_PERIOD_KVARG
> > ("lsc_poll_period_ms")
> > >  #define PMD_BOND_LINK_UP_PROP_DELAY_KVARG      ("up_delay")
> > >  #define PMD_BOND_LINK_DOWN_PROP_DELAY_KVARG
> > ("down_delay")
> > > +#define PMD_BOND_RECV_RING_PKTS                512
> > > +#define PMD_BOND_RECV_PKTS_PER_SLAVE           32
> > >
> > >  #define PMD_BOND_XMIT_POLICY_LAYER2_KVARG      ("l2")
> > >  #define PMD_BOND_XMIT_POLICY_LAYER23_KVARG     ("l23")
> > > @@ -175,6 +177,8 @@ struct bond_dev_private {
> > >
> > >         void *vlan_filter_bmpmem;               /* enabled vlan filter
> > > bitmap */
> > >         struct rte_bitmap *vlan_filter_bmp;
> > > +
> > > +       struct rte_ring *rx_ring;
> > >  };
> > >
> > >  extern const struct eth_dev_ops default_dev_ops;
> > > --
> > > 2.18.0
> > >
> > >