[ovs-dev] [PATCH v10 1/1] Avoid dp_hash recirculation for balance-tcp bond selection mode

[Ilya Maximets]

On 08.01.2020 21:33, Ben Pfaff wrote:
> On Wed, Jan 08, 2020 at 10:48:04AM +0530, Vishal Deep Ajmera wrote:
>> Problem:
>> --------
>> In OVS-DPDK, flows with output over a bond interface of type “balance-tcp”
>> (using a hash on TCP/UDP 5-tuple) get translated by the ofproto layer into
>> "HASH" and "RECIRC" datapath actions. After recirculation, the packet is
>> forwarded to the bond member port based on 8-bits of the datapath hash
>> value computed through dp_hash. This causes performance degradation in the
>> following ways:
> 
> Thanks for the patch!
> 
> I have a few minor stylistic suggestions, see below.
> 
> I'd like to hear Ilya's opinion on this.

Thanks Ben for review!  I'll take another look at this patch in a next
couple of days.

> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 78f9cf928de2..c3a54e96346e 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -378,11 +378,12 @@ struct dp_netdev {
>  
>      struct conntrack *conntrack;
>      struct pmd_auto_lb pmd_alb;
> +
>      /* Bonds.
>       *
>       * Any lookup into 'bonds' requires taking 'bond_mutex'. */
>      struct ovs_mutex bond_mutex;
> -    struct hmap bonds;
> +    struct hmap bonds;          /* Contains "struct tx_bond"s. */
>  };
>  
>  static void meter_lock(const struct dp_netdev *dp, uint32_t meter_id)
> @@ -5581,10 +5582,7 @@ pmd_free_cached_bonds(struct dp_netdev_pmd_thread *pmd)
>  
>      /* Remove bonds from pmd which no longer exists. */
>      HMAP_FOR_EACH_SAFE (bond, next, node, &pmd->bond_cache) {
> -        struct tx_bond *tx = NULL;
> -
> -        tx = tx_bond_lookup(&pmd->tx_bonds, bond->bond_id);
> -        if (!tx) {
> +        if (!tx_bond_lookup(&pmd->tx_bonds, bond->bond_id)) {
>              /* Bond no longer exist. Delete it from pmd. */
>              hmap_remove(&pmd->bond_cache, &bond->node);
>              free(bond);
> @@ -5601,10 +5599,11 @@ pmd_load_cached_bonds(struct dp_netdev_pmd_thread *pmd)
>      pmd_free_cached_bonds(pmd);
>      hmap_shrink(&pmd->bond_cache);
>  
> -    struct tx_bond *tx_bond, *tx_bond_cached;
> +    struct tx_bond *tx_bond;
>      HMAP_FOR_EACH (tx_bond, node, &pmd->tx_bonds) {
>          /* Check if bond already exist on pmd. */
> -        tx_bond_cached = tx_bond_lookup(&pmd->bond_cache, tx_bond->bond_id);
> +        struct tx_bond *tx_bond_cached
> +            = tx_bond_lookup(&pmd->bond_cache, tx_bond->bond_id);
>  
>          if (!tx_bond_cached) {
>              /* Create new bond entry in cache. */
>