On 3/12/25 16:01, Vladislav Odintsov wrote: > > On 12.03.2025 17:45, Ilya Maximets wrote: >> On 3/12/25 12:41, Vladislav Odintsov wrote: >>> On 12.03.2025 14:11, Ilya Maximets wrote: >>>> On 3/12/25 11:22, Vladislav Odintsov wrote: >>>>> Hi Han, Ilya and Adrian, >>>>> >>>>> I'm also suffering from these messages in logs. Han, do you have plans >>>>> on working with requested changes? >>>>> >>>>> If you've got no time at the moment, I can continue this work addressing >>>>> reviewers' comments. Ilya, you said 1kB is a small chunk of data and we >>>>> shouldn't care too much about it. Which value is okay to ignore from >>>>> your perspective? >>>> Are you suffering from excessive logging or from not enough granularity >>>> in the logs? This patch increases the accuracy and doesn't reduce the >>>> amount of logs. >>>> >>>> If the amount of logs is a problem, then maybe we should move the logs >>>> for individual load shifts to DBG level (maybe keep a particularly heavy >>>> ones like >1MB move at INFO) and have a separate overview of the change >>>> instead showing the load distribution after the rebalancing, e.g.: >>>> >>>> bond X: rebalanced (now carrying: portA:XXXkB, portB:YYYkB, portC:ZZZkB) >>>> >>>> For the debug logs we could improve the accuracy as this patch does. >>>> >>>> What do you think? >>> We suffer from excessive logging of "0 kB" records. There are tons of >>> them. So I planned to take your proposal and do not to log these events >>> at all. But I like your last idea about hiding messages of re-balancing >>> <1MB under DBG log level and keep INFO for others. >>> >>> If you guys are okay with that, I can prepare and send a patch. >> I think, if we're hiding some information under DBG, then we need to add >> a new log with an overview of the rebalance with a new load distribution. >> With that, sounds fine to me. > Did you mean that we should log total cumulative re-balanced amount once > in a timeframe (let say 5-15 minutes) or just log with some rate-limit?
The total cumulative amount at the end of ofproto/bond.c:bond_rebalance() if rebalanced == true. See the example log above. So, it the log should appear at most once per rebalance interval, which is 10s by default. The current 'shift X of load' log will remain but at DBG level, except for a very large ones. Will that solve your problem, or is that still too much of logging (once in 10s) ? >> >> Note: chinatelecom.cn mail server doesn't seem to like communicating with >> gmail smtp, and so my replies likely do not reach Han. Let's wait maybe a >> couple days for a reply, in case at least some of these messages are >> delivered. >> >>>> Best regards, Ilya Maximets. >>>> >>>>> On 05.08.2024 10:08, Adrián Moreno wrote: >>>>>> On Fri, Aug 02, 2024 at 06:38:19PM GMT, Ilya Maximets wrote: >>>>>>> On 7/2/24 16:36, Han Ding wrote: >>>>>>>> When the delta is less than 1024 in bond_shift_load, it print "shift >>>>>>>> 0kB of load". >>>>>>>> Like this: >>>>>>>> bond dpdkbond0: shift 0kB of load (with hash 71) from nic1 to nic2 >>>>>>>> (now carrying 20650165kB and 8311662kB load, respectively) >>>>>>> Hi, Han. Thanks for the patch! >>>>>>> >>>>>>> I'm curious, is this information about movements that small >>>>>>> practically useful? >>>>>>> >>>>>> I had the same thought. >>>>>> I guess it should not happen very often given how the algorithm works but >>>>>> OTOH, printing "0kB" is definitely not useful. >>>>>> >>>>>>>> Signed-off-by: Han Ding <[email protected]> >>>>>>>> --- >>>>>>>> ofproto/bond.c | 24 +++++++++++++++++------- >>>>>>>> 1 file changed, 17 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/ofproto/bond.c b/ofproto/bond.c >>>>>>>> index c31869a..5b1975d 100644 >>>>>>>> --- a/ofproto/bond.c >>>>>>>> +++ b/ofproto/bond.c >>>>>>>> @@ -1192,13 +1192,23 @@ 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); >>>>>>>> + if (delta >= 1024) { >>>>>>>> + 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); >>>>>>>> + } else { >>>>>>>> + VLOG_INFO("bond %s: shift %"PRIu64"B of load (with hash >>>>>>>> %"PRIdPTR") " >>>>>>>> + "from %s to %s (now carrying %"PRIu64"kB and " >>>>>> Apart from Ilya's comment, missing one more indentation space in this >>>>>> line (and all others). >>>>>> >>>>>> Thanks. >>>>>> >>>>>>>> + "%"PRIu64"kB load, respectively)", >>>>>>>> + bond->name, delta, hash - bond->hash, >>>>>>>> + from->name, to->name, >>>>>>>> + (from->tx_bytes - delta) / 1024, >>>>>>>> + (to->tx_bytes + delta) / 1024); >>>>>>>> + } >>>>>>> I'd suggest instead of copying the whole thing, just replace "kB" >>>>>>> with another %s and use a couple of ternary operators to produce >>>>>>> correct value and "kB" or "B" depending on the delta. >>>>>>> >>>>>>> Best regards, Ilya Maximets. >>>>>>> _______________________________________________ >>>>>>> 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
