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?
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
