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