On 20.03.2025 23:02, Adrián Moreno wrote:
> On Thu, Mar 20, 2025 at 09:35:22AM -0700, Adrián Moreno wrote:
>> On Mon, Mar 17, 2025 at 09:01:28PM +0100, Ilya Maximets wrote:
>>> On 3/17/25 19:31, Mike Pattrick wrote:
>>>> On Fri, Mar 14, 2025 at 4:06 PM Vladislav Odintsov <[email protected]>
>>>> 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 <[email protected]>
>>>>> Signed-off-by: Vladislav Odintsov <[email protected]>
>>>>> ---
>>>>> 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)",
> I'm also thinking that this log will only show if the bond is
> rebalanced. So just printing the new tx_bytes gives little information
> without knowing their values before. It would be more useful to show
> the delta. WDYT?
delta is a per from-to value. Since bond can have more than two members,
it's quite tricky to print all the information about movements in one
line to a user. And is this information in INFO log level really
needed? While total delta is useless, user may want to see current load
for bond members IMO. I guess DBG log level can be enabled for detailed
information about all movements. WDYT?
>
>>>> "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.
>>>
>> I'm not a native English speaker but the "now carrying" sounds better in
>> the original sentence where bond members are mentioned first, i.e:
>>
>> """
>> bond bond0: shift 42kB of load (with has 0x123) from eth0 to eth1 (now
>> carrying 100kB and 200kB load, respectively)
>> """
>>
>> Than in this one were members are mentioned after:
>>
>> """
>> bond bond0: rebalanced (now carrying: eth0:100kB and eth1:200kB)
>> """
>>
>> In fact, maybe drop the colon after carrying (or whatever other word
>> we choose)?
>>
>> --
>> Adrián
>>
>>>>> + 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
>>>>> [email protected]
>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>>
>>> _______________________________________________
>>> dev mailing list
>>> [email protected]
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
regards,
Vladislav Odintsov
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
