Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Hangbin Liu <liuhangbin@gmail.com>]

On Wed, Jun 15, 2022 at 01:44:57PM -0400, Jonathan Toppins wrote:
> > diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> > index 43be3782e5df..53a18ff7cf23 100644
> > --- a/Documentation/networking/bonding.rst
> > +++ b/Documentation/networking/bonding.rst
> > @@ -780,6 +780,17 @@ peer_notif_delay
> >   	value is 0 which means to match the value of the link monitor
> >   	interval.
> > +prio
> > +	Slave priority. A higher number means higher priority.
> > +	The primary slave has the highest priority. This option also
> > +	follows the primary_reselect rules.
> > +
> > +	This option could only be configured via netlink, and is only valid
> > +	for active-backup(1), balance-tlb (5) and balance-alb (6) mode.
> > +	The valid value range is a signed 32 bit integer.
> > +
> > +	The default value is 0.
> 
> Why is this a signed 32 bit number? Why not a u8, it would seem 256 [255,0]
> options is more than enough to select from. Is there a specific reason it
> needs to be an s32?

The main reason is to compatible with team prio option, which also use a s32
value.

If you think s32 is too wide, how about s16? As u8 looks a little tight.
And follow are the reasons I prefer using singed value.

> 
> If the reason for selecting a signed value is so that the default priority
> could be in the middle of the range, why not just set the default to be 128,
> assuming u8 is wide enough?

First, 128 looks like a weird default value to me as a user.
0/1 as a default looks more reasonable.

Second. If I'm a user, other than using like 111, 125, 128, 130,
I'd prefer to use a multiple of 10 as priority number. e.g. -10, 20, 100, -200.

I know someone prefer a positive value as priority number. But given the
convenience of using negative value for a less wanted slave. I personally
prefer a singed value for priority setting.

Hi, Jay, what do you think?

> > @@ -157,6 +162,20 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> >   			return err;
> >   	}
> > +	if (data[IFLA_BOND_SLAVE_PRIO]) { > +		int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> > +		char prio_str[IFNAMSIZ + 7];
> > +
> > +		/* prio option setting expects slave_name:prio */
> > +		snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
> > +			 slave_dev->name, prio);
> > +
> > +		bond_opt_initstr(&newval, prio_str);
> 
> It might be less code and a little cleaner to extend struct bond_opt_value
> with a slave pointer.
> 
> 	struct bond_opt_value {
> 		char *string;
> 		u64 value;
> 		u32 flags;
> 		union {
> 			char cextra[BOND_OPT_EXTRA_MAXLEN];
> 			struct net_device *slave_dev;
> 		} extra;
> 	};
> 
> Then modify __bond_opt_init to set the slave pointer, basically a set of
> bond_opt_slave_init{} macros. This would remove the need to parse the slave
> interface name in the set function. Setting .flags = BOND_OPTFLAG_RAWVAL
> (already done I see) in the option definition to avoid bond_opt_parse() from
> loosing our extra information by pointing to a .values table entry. Now in
> the option specific set function we can just find the slave entry and set
> the value, no more string parsing code needed.

This looks reasonable to me. It would make all slave options setting easier
for future usage.

> 
> > +		err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval);
> 
> I think this patch series needs to be rebased onto latest net-next/master as
> a patch series I sent added two extra parameter list arguments to
> __bond_opt_set().

OK, I will.

> 
>   2bff369b2354 bonding: netlink error message support for options
> 
> Considering my comments above about extending bond_opt_value, I might look
> as sending a fixup patch to remove all the parameter list additions and hide
> the netlink extack pointer in bond_opt_value.
> 
> > +		if (err)
> > +			return err;
> > +	}
> > +
> >   	return 0;
> >   }
> > @@ -1306,6 +1318,61 @@ static int bond_option_missed_max_set(struct bonding *bond,
> >   	return 0;
> >   }
> > +static int bond_option_prio_set(struct bonding *bond,
> > +				const struct bond_opt_value *newval)
> > +{
> > +	struct slave *slave, *update_slave;
> > +	struct net_device *sdev;
> > +	struct list_head *iter;
> > +	char *delim;
> > +	int ret = 0;
> > +	int prio;
> 
> Priorities are only considered if there is no primary set, correct? Would
> you not want to issue a netdev_warn here noting the fact that this will be
> ignored if the bond device has a primary set? Much like how other options
> issue warnings, or in miimon's case turn off arp monitor, when other
> configuration influence the effectiveness of the setting?

OK, I will.

Thanks
Hangbin