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

Ilya Maximets i.maximets at ovn.org
Tue Mar 31 19:35:35 UTC 2020


On 3/31/20 8:57 PM, Ilya Maximets wrote:
> On 3/11/20 3:15 PM, Vishal Deep Ajmera wrote:
>> @@ -6304,6 +6431,53 @@ dp_netdev_del_port_tx_from_pmd(struct dp_netdev_pmd_thread *pmd,
>>      free(tx);
>>      pmd->need_reload = true;
>>  }
>> +
>> +/* Add bond to the tx bond cmap of 'pmd'. */
>> +static void
>> +dp_netdev_add_bond_tx_to_pmd(struct dp_netdev_pmd_thread *pmd,
>> +                             struct tx_bond *bond)
>> +{
>> +    struct tx_bond *tx;
>> +
>> +    ovs_mutex_lock(&pmd->bond_mutex);
>> +    tx = tx_bond_lookup(&pmd->tx_bonds, bond->bond_id);
>> +    if (tx) {
>> +        struct tx_bond *new_tx = xmemdup(bond, sizeof *bond);
>> +
>> +        /* Copy the stats for each bucket. */
>> +        for (int i = 0; i < BOND_BUCKETS; i++) {
>> +            uint64_t n_packets, n_bytes;
>> +
>> +            atomic_read_relaxed(&tx->slave_buckets[i].n_packets, &n_packets);
>> +            atomic_read_relaxed(&tx->slave_buckets[i].n_bytes, &n_bytes);
>> +            atomic_init(&new_tx->slave_buckets[i].n_packets, n_packets);
>> +            atomic_init(&new_tx->slave_buckets[i].n_bytes, n_bytes);
> 
> Actually, why we're copying stats here?
> 
> This function is called directly from dpif_netdev_bond_add() which is called
> from update_recirc_rules__() after boning rebalancing.  This means that bucket
> to slave mapping is likely changed.  This code copies bucket stats from the
> previous state where it was related to a different slave.
> Moreover, even if it was the same slave, bond_recirculation_account() always
> adds stats received from flow/datapath, i.e. same stats will be incrementally
> accounted over and over again messing up slave statistics.
> AFAIU, recirculation rules are always brand new, i.e. their datapath stats are
> zero after each rebalancing, the same should be here.
> 
> Could you, please, check that slave statistics makes any sence on rebalancing?
> i.e. it is realistic or it's increased way to fast?  I bet there is an issue here.

Sorry, this one comment is not correct.  I missed the fact that delta is counted before
adding new stats.  And, actually, flows are modified instead of re-adding, which makes
more sense.  So, this should be OK, please ignore.  Need more coffee. :)


More information about the dev mailing list