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