[ovs-dev] [PATCH v4] AB bonding: Add "primary" interface concept

[Flavio Leitner]

On Thu, May 21, 2020 at 05:56:27PM +0000, Jeff Squyres (jsquyres) wrote:
> On May 13, 2020, at 11:07 AM, Flavio Leitner <fbl at sysclose.org> wrote:
> > I wonder if we could store a bond_slave pointer into struct bond
> > instead of a string, because then we don't need the bool on each
> > bond_slave to indicate if it's primary. It seems redundant to me.
> 
> I chose to keep the string name of the primary interface on the bond struct itself for a few reasons:
> 
> 1. The primary interface name originally came from the bond_settings struct when the bond was initially created, which was filled from the database.  The bond_settings struct is not saved/cached anywhere; values that need to be kept are copied to the bond struct.  Saving the primary interface name to the bond struct seemed in keeping with that convention.
> 
> 2. The name of the primary interface is set in the database; it's durable on the bond, even if slave interfaces are subsequently registered or unregistered.  It seemed simpler to cache this string value on the bond so that when an interface is added to a bond, we can do an easy strcmp to see if it's now the primary interface (vs. having to go retrieve it out of the database before doing the strcmp).
> 
> 3. I thought about removing the primary interface name from the database if the primary interface was removed from the bond, but that seemed pretty complicated, and I didn't know if that was a common thing to do.
> 
> That being said, between:
> 
> A. Setting slave->is_primary in bond_slave_register() and checking slave->is_primary in the existing slave loop in bond_run()
> 
> or
> 
> B. Setting bond->pointer_to_primary_slave in bond_slave_register(), resetting bond->pointer_to_primary_slave=NULL in bond_slave_unregister(), and checking bond->pointer_to_primary outside the loop in bond_run()
> 
> I don't have much of an opinion.  The string name still needs to be maintained on the bond struct either way, I think.
> 
> FWIW: adding the (struct bond_slave.is_primary) field probably did not grow the size of the struct; it was added in an alignment-induced gap between (bool may_enable) and (long long delay_expires).
> 
> Do you feel strongly / want me to change it?

You're probably right. I will wait for the next version of the
patchset and check again. Maybe the removal part changes something.

Thanks!
-- 
fbl