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?
>
> 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 <hand...@chinatelecom.cn>
>>>>>>> ---
>>>>>>> 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
>>>>>> 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
--
Regards,
Vladislav Odintsov
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
