Subject: Re: [patch net-next v3 11/15] netdevsim: implement proper devlink reload

From: Jiri Pirko <[email-masked]>
Date: Fri, 4 Oct 2019 08:19:14 +0200

Fri, Oct 04, 2019 at 01:17:30AM CEST, [email-masked] wrote:
>On Thu,  3 Oct 2019 11:49:36 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <[email-masked]>
>> 
>> During devlink reload, all driver objects should be reinstantiated with
>> the exception of devlink instance and devlink resources and params.
>> Move existing devlink_resource_size_get() calls into fib_create() just
>> before fib notifier is registered. Also, make sure that extack is
>> propagated down to fib_notifier_register() call.
>> 
>> Signed-off-by: Jiri Pirko <[email-masked]>
>
>Acked-by: Jakub Kicinski <[email-masked]>
>
>> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
>> index d2aeac0f4c2c..fdc682f3a09a 100644
>> --- a/drivers/net/netdevsim/fib.c
>> +++ b/drivers/net/netdevsim/fib.c
>> @@ -63,12 +63,10 @@ u64 nsim_fib_get_val(struct nsim_fib_data *fib_data,
>>  	return max ? entry->max : entry->num;
>>  }
>>  
>> -int nsim_fib_set_max(struct nsim_fib_data *fib_data,
>> -		     enum nsim_resource_id res_id, u64 val,
>> -		     struct netlink_ext_ack *extack)
>> +static void nsim_fib_set_max(struct nsim_fib_data *fib_data,
>> +			     enum nsim_resource_id res_id, u64 val)
>>  {
>>  	struct nsim_fib_entry *entry;
>> -	int err = 0;
>>  
>>  	switch (res_id) {
>>  	case NSIM_RESOURCE_IPV4_FIB:
>> @@ -84,20 +82,10 @@ int nsim_fib_set_max(struct nsim_fib_data *fib_data,
>>  		entry = &fib_data->ipv6.rules;
>>  		break;
>>  	default:
>> -		return 0;
>> -	}
>> -
>> -	/* not allowing a new max to be less than curren occupancy
>> -	 * --> no means of evicting entries
>> -	 */
>> -	if (val < entry->num) {
>> -		NL_SET_ERR_MSG_MOD(extack, "New size is less than current occupancy");
>> -		err = -EINVAL;
>
>This change in behaviour should perhaps be mentioned in the commit
>message. The reload will no longer fail if the resources are
>insufficient. 

Reload is going to fail if the resources are insufficient. I have a
selftest for that, please see the last patch.

>
>Since we want to test reload more widely than just for the FIB limits
>that does make sense to me. Is that the thinking?
>
>> -	} else {
>> -		entry->max = val;
>> +		WARN_ON(1);
>> +		return;
>>  	}
>> -
>> -	return err;
>> +	entry->max = val;
>>  }
>>  
>>  static int nsim_fib_rule_account(struct nsim_fib_entry *entry, bool add,