On 3/17/25 19:31, Mike Pattrick wrote:
> On Fri, Mar 14, 2025 at 4:06 PM Vladislav Odintsov <vlodintsov@k2.cloud>
> wrote:
>>
>> Rebalancing stats are printed once in a run with INFO log level.
>> Old detailed per-hash rebalancing stats now printed with DBG log level.
>>
>> Reported-at:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2025-March/422028.html
>> Suggested-by: Ilya Maximets <i.maxim...@ovn.org>
>> Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud>
>> ---
>> ofproto/bond.c | 35 ++++++++++++++++++++++++++---------
>> 1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/ofproto/bond.c b/ofproto/bond.c
>> index 45a36fabb..17bf10be5 100644
>> --- a/ofproto/bond.c
>> +++ b/ofproto/bond.c
>> @@ -1197,13 +1197,13 @@ bond_shift_load(struct bond_entry *hash, struct
>> bond_member *to)
>> struct bond *bond = from->bond;
>> uint64_t delta = hash->tx_bytes;
>>
>> - VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") "
>> - "from %s to %s (now carrying %"PRIu64"kB and "
>> - "%"PRIu64"kB load, respectively)",
>> - bond->name, delta / 1024, hash - bond->hash,
>> - from->name, to->name,
>> - (from->tx_bytes - delta) / 1024,
>> - (to->tx_bytes + delta) / 1024);
>> + VLOG_DBG("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") "
>> + "from %s to %s (now carrying %"PRIu64"kB and "
>> + "%"PRIu64"kB load, respectively)",
>> + bond->name, delta / 1024, hash - bond->hash,
>> + from->name, to->name,
>> + (from->tx_bytes - delta) / 1024,
>> + (to->tx_bytes + delta) / 1024);
>>
>> /* Shift load away from 'from' to 'to'. */
>> from->tx_bytes -= delta;
>> @@ -1434,8 +1434,25 @@ bond_rebalance(struct bond *bond)
>> e->tx_bytes /= 2;
>> }
>>
>> - if (use_recirc && rebalanced) {
>> - bond_update_post_recirc_rules__(bond,true);
>> + if (rebalanced) {
>> + int i = 0;
>> + struct ds member_stats;
>> + ds_init(&member_stats);
Reverse x-mass tree. And use the DS_EMPTY_INITIALIZER, since it's an
initialization at the point of declaration.
>
> All of the log related code should be wrapped in a if(VLOG_IS_INFO_ENABLED())
The INFO is a default level, there are not many good use cases for
VLOG_IS_INFO_ENABLED and higher. In fact, I'm not sure if the only use of
it we have in OVS is justified.
Besides, the calculation below are not really heavy. And are cheaper than
some of the other code in this function.
>
>> +
>> + HMAP_FOR_EACH (member, hmap_node, &bond->members) {
>> + if (++i > 1) {
>> + ds_put_cstr(&member_stats, " and ");
>> + }
>> + ds_put_format(&member_stats, "%s:%"PRIu64"kB", member->name,
>> + member->tx_bytes / 1024);
>> + }
>> + VLOG_INFO("bond %s: rebalanced (now carrying: %s)",
>
> "now carrying:" could probably be changed to something like "current load"
'now carrying' highlights the fact that it's the load after rebalancing.
May be easier to read together with the debug messages if the debug logging
is enabled. May also be better to follow wording from the existing messages
as it's more familiar to users and it may be easier to grep for (the word
'carrying' appears only in bonding logs). But I'm not sure.
>
>> + bond->name, member_stats.string);
>> + ds_destroy(&member_stats);
>> +
>> + if (use_recirc) {
>> + bond_update_post_recirc_rules__(bond, true);
>> + }
>> }
>>
>> done:
>> --
>> 2.48.1
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
