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?
> > >
> > > "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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
