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 <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.
>
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
> >> 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
