diff mbox series

[net-next,v3,11/15] netdevsim: implement proper devlink reload

Message ID 20191003094940.9797-12-jiri@resnulli.us
State Accepted
Delegated to: David Miller
Headers show
Series devlink: allow devlink instances to change network namespace | expand

Commit Message

Jiri Pirko Oct. 3, 2019, 9:49 a.m. UTC 
From: Jiri Pirko <jiri@mellanox.com>

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 <jiri@mellanox.com>
---
v2->v3:
- removed extra empty line in nsim_dev_reload_up()
- did separate create/destroy functions for reload and non-reload cases
v1->v2:
- don't reinstantiate debugfs during reload.
---
 drivers/net/netdevsim/dev.c       | 95 ++++++++++++++++++++++---------
 drivers/net/netdevsim/fib.c       | 53 +++++++++--------
 drivers/net/netdevsim/netdevsim.h |  8 +--
 3 files changed, 99 insertions(+), 57 deletions(-)

Comments

Jakub Kicinski Oct. 3, 2019, 11:17 p.m. UTC | #1 
On Thu,  3 Oct 2019 11:49:36 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> 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 <jiri@mellanox.com>

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>

> 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. 

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,
Jiri Pirko Oct. 4, 2019, 6:19 a.m. UTC | #2 
Fri, Oct 04, 2019 at 01:17:30AM CEST, jakub.kicinski@netronome.com wrote:
>On Thu,  3 Oct 2019 11:49:36 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> 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 <jiri@mellanox.com>
>
>Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
>> 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,
Jakub Kicinski Oct. 4, 2019, 5:42 p.m. UTC | #3 
On Fri, 4 Oct 2019 08:19:14 +0200, Jiri Pirko wrote:
> >> @@ -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.

Oh, because re-registering the fib notifier itself will fail? 
All good then, thanks.

> >Since we want to test reload more widely than just for the FIB limits
> >that does make sense to me. Is that the thinking?
Jiri Pirko Oct. 4, 2019, 8:49 p.m. UTC | #4 
Fri, Oct 04, 2019 at 07:42:17PM CEST, jakub.kicinski@netronome.com wrote:
>On Fri, 4 Oct 2019 08:19:14 +0200, Jiri Pirko wrote:
>> >> @@ -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.
>
>Oh, because re-registering the fib notifier itself will fail? 

Yep.


>All good then, thanks.
>
>> >Since we want to test reload more widely than just for the FIB limits
>> >that does make sense to me. Is that the thinking?
diff mbox series

Patch

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 3cc101aee991..7de80faab047 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -469,9 +469,16 @@  static void nsim_dev_traps_exit(struct devlink *devlink)
 	kfree(nsim_dev->trap_data);
 }
 
+static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
+				  struct netlink_ext_ack *extack);
+static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev);
+
 static int nsim_dev_reload_down(struct devlink *devlink,
 				struct netlink_ext_ack *extack)
 {
+	struct nsim_dev *nsim_dev = devlink_priv(devlink);
+
+	nsim_dev_reload_destroy(nsim_dev);
 	return 0;
 }
 
@@ -479,27 +486,8 @@  static int nsim_dev_reload_up(struct devlink *devlink,
 			      struct netlink_ext_ack *extack)
 {
 	struct nsim_dev *nsim_dev = devlink_priv(devlink);
-	enum nsim_resource_id res_ids[] = {
-		NSIM_RESOURCE_IPV4_FIB, NSIM_RESOURCE_IPV4_FIB_RULES,
-		NSIM_RESOURCE_IPV6_FIB, NSIM_RESOURCE_IPV6_FIB_RULES
-	};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(res_ids); ++i) {
-		int err;
-		u64 val;
-
-		err = devlink_resource_size_get(devlink, res_ids[i], &val);
-		if (!err) {
-			err = nsim_fib_set_max(nsim_dev->fib_data,
-					       res_ids[i], val, extack);
-			if (err)
-				return err;
-		}
-	}
-	nsim_devlink_param_load_driverinit_values(devlink);
 
-	return 0;
+	return nsim_dev_reload_create(nsim_dev, extack);
 }
 
 #define NSIM_DEV_FLASH_SIZE 500000
@@ -687,8 +675,49 @@  static int nsim_dev_port_add_all(struct nsim_dev *nsim_dev,
 	return err;
 }
 
-static struct nsim_dev *
-nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
+static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
+				  struct netlink_ext_ack *extack)
+{
+	struct nsim_bus_dev *nsim_bus_dev = nsim_dev->nsim_bus_dev;
+	struct devlink *devlink;
+	int err;
+
+	devlink = priv_to_devlink(nsim_dev);
+	nsim_dev = devlink_priv(devlink);
+	INIT_LIST_HEAD(&nsim_dev->port_list);
+	mutex_init(&nsim_dev->port_list_lock);
+	nsim_dev->fw_update_status = true;
+
+	nsim_dev->fib_data = nsim_fib_create(devlink, extack);
+	if (IS_ERR(nsim_dev->fib_data))
+		return PTR_ERR(nsim_dev->fib_data);
+
+	nsim_devlink_param_load_driverinit_values(devlink);
+
+	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
+	if (err)
+		goto err_fib_destroy;
+
+	err = nsim_dev_traps_init(devlink);
+	if (err)
+		goto err_dummy_region_exit;
+
+	err = nsim_dev_port_add_all(nsim_dev, nsim_bus_dev->port_count);
+	if (err)
+		goto err_traps_exit;
+
+	return 0;
+
+err_traps_exit:
+	nsim_dev_traps_exit(devlink);
+err_dummy_region_exit:
+	nsim_dev_dummy_region_exit(nsim_dev);
+err_fib_destroy:
+	nsim_fib_destroy(devlink, nsim_dev->fib_data);
+	return err;
+}
+
+static struct nsim_dev *nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
 {
 	struct nsim_dev *nsim_dev;
 	struct devlink *devlink;
@@ -711,7 +740,7 @@  nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
 	if (err)
 		goto err_devlink_free;
 
-	nsim_dev->fib_data = nsim_fib_create(devlink);
+	nsim_dev->fib_data = nsim_fib_create(devlink, NULL);
 	if (IS_ERR(nsim_dev->fib_data)) {
 		err = PTR_ERR(nsim_dev->fib_data);
 		goto err_resources_unregister;
@@ -772,21 +801,31 @@  nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
 	return ERR_PTR(err);
 }
 
-static void nsim_dev_destroy(struct nsim_dev *nsim_dev)
+static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
 {
 	struct devlink *devlink = priv_to_devlink(nsim_dev);
 
+	if (devlink_is_reload_failed(devlink))
+		return;
 	nsim_dev_port_del_all(nsim_dev);
-	nsim_bpf_dev_exit(nsim_dev);
-	nsim_dev_debugfs_exit(nsim_dev);
 	nsim_dev_traps_exit(devlink);
 	nsim_dev_dummy_region_exit(nsim_dev);
+	mutex_destroy(&nsim_dev->port_list_lock);
+	nsim_fib_destroy(devlink, nsim_dev->fib_data);
+}
+
+static void nsim_dev_destroy(struct nsim_dev *nsim_dev)
+{
+	struct devlink *devlink = priv_to_devlink(nsim_dev);
+
+	nsim_dev_reload_destroy(nsim_dev);
+
+	nsim_bpf_dev_exit(nsim_dev);
+	nsim_dev_debugfs_exit(nsim_dev);
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
 	devlink_unregister(devlink);
-	nsim_fib_destroy(devlink, nsim_dev->fib_data);
 	devlink_resources_unregister(devlink, NULL);
-	mutex_destroy(&nsim_dev->port_list_lock);
 	devlink_free(devlink);
 }
 
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;
-	} 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,
@@ -239,7 +227,28 @@  static u64 nsim_fib_ipv6_rules_res_occ_get(void *priv)
 	return nsim_fib_get_val(data, NSIM_RESOURCE_IPV6_FIB_RULES, false);
 }
 
-struct nsim_fib_data *nsim_fib_create(struct devlink *devlink)
+static void nsim_fib_set_max_all(struct nsim_fib_data *data,
+				 struct devlink *devlink)
+{
+	enum nsim_resource_id res_ids[] = {
+		NSIM_RESOURCE_IPV4_FIB, NSIM_RESOURCE_IPV4_FIB_RULES,
+		NSIM_RESOURCE_IPV6_FIB, NSIM_RESOURCE_IPV6_FIB_RULES
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(res_ids); i++) {
+		int err;
+		u64 val;
+
+		err = devlink_resource_size_get(devlink, res_ids[i], &val);
+		if (err)
+			val = (u64) -1;
+		nsim_fib_set_max(data, res_ids[i], val);
+	}
+}
+
+struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
+				      struct netlink_ext_ack *extack)
 {
 	struct nsim_fib_data *data;
 	int err;
@@ -248,15 +257,11 @@  struct nsim_fib_data *nsim_fib_create(struct devlink *devlink)
 	if (!data)
 		return ERR_PTR(-ENOMEM);
 
-	data->ipv4.fib.max = (u64)-1;
-	data->ipv4.rules.max = (u64)-1;
-
-	data->ipv6.fib.max = (u64)-1;
-	data->ipv6.rules.max = (u64)-1;
+	nsim_fib_set_max_all(data, devlink);
 
 	data->fib_nb.notifier_call = nsim_fib_event_nb;
 	err = register_fib_notifier(&init_net, &data->fib_nb,
-				    nsim_fib_dump_inconsistent, NULL);
+				    nsim_fib_dump_inconsistent, extack);
 	if (err) {
 		pr_err("Failed to register fib notifier\n");
 		goto err_out;
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index ac506cf253b6..702d951fe160 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -173,13 +173,11 @@  int nsim_dev_port_add(struct nsim_bus_dev *nsim_bus_dev,
 int nsim_dev_port_del(struct nsim_bus_dev *nsim_bus_dev,
 		      unsigned int port_index);
 
-struct nsim_fib_data *nsim_fib_create(struct devlink *devlink);
-void nsim_fib_destroy(struct devlink *devlink, struct nsim_fib_data *data);
+struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
+				      struct netlink_ext_ack *extack);
+void nsim_fib_destroy(struct devlink *devlink, struct nsim_fib_data *fib_data);
 u64 nsim_fib_get_val(struct nsim_fib_data *fib_data,
 		     enum nsim_resource_id res_id, bool max);
-int nsim_fib_set_max(struct nsim_fib_data *fib_data,
-		     enum nsim_resource_id res_id, u64 val,
-		     struct netlink_ext_ack *extack);
 
 #if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
 void nsim_ipsec_init(struct netdevsim *ns);