Hi all：
Thank you for your attention to this issue.
Initially, we noticed that bond_shift_load triggered
bond->bond_revalidate = true, which then led to bond_reconfigure ->
bond_entry_reset being executed. This ultimately caused
USERSPACE_INVALID_PORT_DROP to occur.
We believe that no packet loss should happen during traffic load
balancing.
Later, we discovered that changes to the bond-rebalance-interval setting
could also lead to the same issue, which is why we reported the problem.
In fact, we are more concerned about the packet drops caused by
bond_shift_load. If the packet loss due to bond-rebalance-interval changes is
truly unavoidable, we can accept that.
Apologies for not describing the issue clearly enough earlier.
chenyongch...@cestc.cn
发件人： Ilya Maximets
发送时间： 2025-08-01 04:48
收件人： Aaron Conole
抄送： i.maximets; dev; mhou; chenyongchang; Adrian Moreno
主题： Re: [ovs-dev] [PATCH] bond: Do not flag a rebalance when adjusting
time.【请注意，邮件由i.maximets....@gmail.com代发】
On 7/31/25 8:41 PM, Aaron Conole wrote:
> Ilya Maximets <i.maxim...@ovn.org> writes:
>
>> On 7/30/25 4:47 PM, Aaron Conole via dev wrote:
>>> When adjusting bond parameters, any adjustment is considered sufficient
>>> for triggering a rebalance. This is a very simplistic config update
>>> scheme that triggers complete rebalancing even if the time adjustment
>>> would move the next expiration out beyond the last calculated expiration.
>>>
>>> For the interval parameter only, we can simply recalculate the expiry
>>> deadline and let the next bond_run() event do the rebalance if needed.
>>> Even if the recalculation would cause the deadline to have occurred in
>>> the past, it should execute on the next bond_run() anyway. This is
>>> still okay, as the rebalance interval timeout may not result in a
>>> full rebalance anyway.
>>>
>>> Reported-at:
>>> https://www.mail-archive.com/ovs-discuss@openvswitch.org/msg10409.html
>>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>>> ---
>>> ofproto/bond.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/bond.c b/ofproto/bond.c
>>> index 3859ddca08..86e21607e5 100644
>>> --- a/ofproto/bond.c
>>> +++ b/ofproto/bond.c
>>> @@ -459,8 +459,14 @@ bond_reconfigure(struct bond *bond, const struct
>>> bond_settings *s)
>>> }
>>>
>>> if (bond->rebalance_interval != s->rebalance_interval) {
>>> + /* Recompute the next rebalance interval by moving the
>>> next_rebalance
>>> + * to be offset by the new interval. Then let the rebalance code
>>> + * trigger a rebalance based on the new details.
>>
>> This sentence doesn't seem necessary.
>
> Removed.
>
>>> In this case, if
>>> + * all that was updated is the rebalance interval, we can skip
>>> + * triggering the rest of the port reconfigure mechanism. */
>>
>> s/reconfigure mechanism/reconfiguration/
>
> Ack.
>
>>> + int old_start_time = bond->next_rebalance -
>>> bond->rebalance_interval;
>>> bond->rebalance_interval = s->rebalance_interval;
>>> - revalidate = true;
>>> + bond->next_rebalance = old_start_time + bond->rebalance_interval;
>>> }
>>
>> We should likely still revalidate when the value goes to or from zero,
>> because that impacts if the bond considered balanced or not. Even if
>> that doesn't cause any problems today, it will inevitably be forgotten
>> in the future.
>
> That makes sense to me - I'll add a case for it.
>
>> Also, the subtraction may technically overflow if the next_rebalance is
>> not initialized. E.g. if it is not initialized yet when the bond is
>> going from active-backup to balance-tcp. Which may be fine, given those
>> are signed integers, but still doesn't feel right.
>
> I'm trying to envision a scenario that would make this happen. For
> example, it gets reset when going from ab -> balance-{tcp,slb}. And
> that will be reconfigured later in this very code path as well (since we
> will call the bond_entry_reset if the config got changed for any config
> option other than the interval). I can put a guard on !next_rebalance
> and tell it to trigger revalidation then (since the very first execution
> or something should be initialized to '0' and obviously that could
> probably find a trigger). But still struggling to see a place where
> changing bond mode won't also force the whole thing to be reinitialized
> in bond_entry_reset() since revalidate will be 'true' via other means.
It doesn't cause any issues, I agree, but it just didn't feel right. :)
Feel free to keep this code as-is.
>
>> And this patch needs a Fixes tag.
>
> I didn't put one because I'm not sure if it's really a bugfix or an
> improvement. If you consider it one, I'll add the fixes tag.
Looking closer to the code, there are two separate issues here:
1. Traffic is re-shuffled among bond members on the rebalance interval
change.
2. Traffic is being dropped for some time after the bond is reconfigured
until the next bond_run(). This happens if other config options
are changed as well.
And, I think, the latter is what the original reporter was concerned about.
This is a behavior introduced in:
586adfd047cb ("bond: Avoid deadlock while updating post recirculation rules.")
Before that change we would not update post-recirc rules while the
configuration is cleared and not re-constructed yet. But after the
586adfd047cb fix, we're now calling the update after we zeroed out the
configuration, effectively just removing all the post recirculation rules
or the lb-output mappings.
Saying that, maybe the real fix is to use bond_update_post_recirc_rules__
with force instead of direct call to update_recirc_rules(). That will
ensure that the configuration is not blank and we actually have real
post-recirc rules / lb-output mapping at all times. I do not remember
why I went with the direct call in that fix though.
What do you think?
Not re-setting the config on timeout change is still needed, as that
avoids re-shuffling the traffic among bond members (issue 1).
The behavior in point 1 was always this way, AFAIU, so that can be treated
as just an enhancement, I suppose.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
