diff mbox series

[net-next,01/10] netdevsim: fib: Convert the current occupancy to an atomic variable

Message ID 20210126132311.3061388-2-idosch@idosch.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add notifications when route hardware flags change | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 3 of 3 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 100 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Ido Schimmel Jan. 26, 2021, 1:23 p.m. UTC 
From: Amit Cohen <amcohen@nvidia.com>

When route is added/deleted, the appropriate counter is increased/decreased
to maintain number of routes.

User can limit the number of routes and then according to the appropriate
counter, adding more routes than the limitation is forbidden.

Currently, there is one lock which protects hashtable, list and accounting.

Handling the counters will be performed from both atomic context and
non-atomic context, while the hashtable and the list will be used only from
non-atomic context and therefore will be protected by a separate lock.

Protect accounting by using an atomic variable, so lock is not needed.

Signed-off-by: Amit Cohen <amcohen@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/netdevsim/fib.c | 56 ++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

Comments

David Ahern Jan. 27, 2021, 4:33 a.m. UTC | #1 
On 1/26/21 6:23 AM, Ido Schimmel wrote:
> @@ -889,22 +882,29 @@ static void nsim_nexthop_destroy(struct nsim_nexthop *nexthop)
>  static int nsim_nexthop_account(struct nsim_fib_data *data, u64 occ,
>  				bool add, struct netlink_ext_ack *extack)
>  {
> -	int err = 0;
> +	int i, err = 0;
>  
>  	if (add) {
> -		if (data->nexthops.num + occ <= data->nexthops.max) {
> -			data->nexthops.num += occ;
> -		} else {
> -			err = -ENOSPC;
> -			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
> -		}
> +		for (i = 0; i < occ; i++)
> +			if (!atomic64_add_unless(&data->nexthops.num, 1,
> +						 data->nexthops.max)) {

seems like this can be
		if (!atomic64_add_unless(&data->nexthops.num, occ,
					 data->nexthops.max)) {

and then the err_num_decrease is not needed

> +				err = -ENOSPC;
> +				NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
> +				goto err_num_decrease;
> +			}
>  	} else {
> -		if (WARN_ON(occ > data->nexthops.num))
> +		if (WARN_ON(occ > atomic64_read(&data->nexthops.num)))
>  			return -EINVAL;
> -		data->nexthops.num -= occ;
> +		atomic64_sub(occ, &data->nexthops.num);
>  	}
>  
>  	return err;
> +
> +err_num_decrease:
> +	for (i--; i >= 0; i--)
> +		atomic64_dec(&data->nexthops.num);

and if this path is really needed, why not atomic64_sub here?

> +	return err;
> +
>  }
>  
>  static int nsim_nexthop_add(struct nsim_fib_data *data,
>
Amit Cohen Jan. 27, 2021, 10:51 a.m. UTC | #2 
>-----Original Message-----
>From: David Ahern <dsahern@gmail.com>
>Sent: Wednesday, January 27, 2021 6:33
>To: Ido Schimmel <idosch@idosch.org>; netdev@vger.kernel.org
>Cc: davem@davemloft.net; kuba@kernel.org; Amit Cohen <amcohen@nvidia.com>; Roopa Prabhu <roopa@nvidia.com>; Donald
>Sharp <sharpd@nvidia.com>; Benjamin Poirier <bpoirier@nvidia.com>; mlxsw <mlxsw@nvidia.com>; Ido Schimmel
><idosch@nvidia.com>
>Subject: Re: [PATCH net-next 01/10] netdevsim: fib: Convert the current occupancy to an atomic variable
>
>On 1/26/21 6:23 AM, Ido Schimmel wrote:
>> @@ -889,22 +882,29 @@ static void nsim_nexthop_destroy(struct
>> nsim_nexthop *nexthop)  static int nsim_nexthop_account(struct nsim_fib_data *data, u64 occ,
>>  				bool add, struct netlink_ext_ack *extack)  {
>> -	int err = 0;
>> +	int i, err = 0;
>>
>>  	if (add) {
>> -		if (data->nexthops.num + occ <= data->nexthops.max) {
>> -			data->nexthops.num += occ;
>> -		} else {
>> -			err = -ENOSPC;
>> -			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
>> -		}
>> +		for (i = 0; i < occ; i++)
>> +			if (!atomic64_add_unless(&data->nexthops.num, 1,
>> +						 data->nexthops.max)) {
>
>seems like this can be
>		if (!atomic64_add_unless(&data->nexthops.num, occ,
>					 data->nexthops.max)) {

atomic64_add_unless(x, y, z) adds y to x if x was not already z.
Which means that when for example num=2, occ=2, max=3:
atomic64_add_unless(&data->nexthops.num, occ, data->nexthops.max) won't fail when it should.

This situation is realistic and actually with atomic64_add_unless() selftests/drivers/net/netdevsim/nexthop.sh fails.

>
>and then the err_num_decrease is not needed
>
>> +				err = -ENOSPC;
>> +				NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
>> +				goto err_num_decrease;
>> +			}
>>  	} else {
>> -		if (WARN_ON(occ > data->nexthops.num))
>> +		if (WARN_ON(occ > atomic64_read(&data->nexthops.num)))
>>  			return -EINVAL;
>> -		data->nexthops.num -= occ;
>> +		atomic64_sub(occ, &data->nexthops.num);
>>  	}
>>
>>  	return err;
>> +
>> +err_num_decrease:
>> +	for (i--; i >= 0; i--)
>> +		atomic64_dec(&data->nexthops.num);
>
>and if this path is really needed, why not atomic64_sub here?
>
>> +	return err;
>> +
>>  }
>>
>>  static int nsim_nexthop_add(struct nsim_fib_data *data,
>>
David Ahern Jan. 28, 2021, 3:42 a.m. UTC | #3 
On 1/27/21 3:51 AM, Amit Cohen wrote:
> 
> 
>> -----Original Message-----
>> From: David Ahern <dsahern@gmail.com>
>> Sent: Wednesday, January 27, 2021 6:33
>> To: Ido Schimmel <idosch@idosch.org>; netdev@vger.kernel.org
>> Cc: davem@davemloft.net; kuba@kernel.org; Amit Cohen <amcohen@nvidia.com>; Roopa Prabhu <roopa@nvidia.com>; Donald
>> Sharp <sharpd@nvidia.com>; Benjamin Poirier <bpoirier@nvidia.com>; mlxsw <mlxsw@nvidia.com>; Ido Schimmel
>> <idosch@nvidia.com>
>> Subject: Re: [PATCH net-next 01/10] netdevsim: fib: Convert the current occupancy to an atomic variable
>>
>> On 1/26/21 6:23 AM, Ido Schimmel wrote:
>>> @@ -889,22 +882,29 @@ static void nsim_nexthop_destroy(struct
>>> nsim_nexthop *nexthop)  static int nsim_nexthop_account(struct nsim_fib_data *data, u64 occ,
>>>  				bool add, struct netlink_ext_ack *extack)  {
>>> -	int err = 0;
>>> +	int i, err = 0;
>>>
>>>  	if (add) {
>>> -		if (data->nexthops.num + occ <= data->nexthops.max) {
>>> -			data->nexthops.num += occ;
>>> -		} else {
>>> -			err = -ENOSPC;
>>> -			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
>>> -		}
>>> +		for (i = 0; i < occ; i++)
>>> +			if (!atomic64_add_unless(&data->nexthops.num, 1,
>>> +						 data->nexthops.max)) {
>>
>> seems like this can be
>> 		if (!atomic64_add_unless(&data->nexthops.num, occ,
>> 					 data->nexthops.max)) {
> 
> atomic64_add_unless(x, y, z) adds y to x if x was not already z.
> Which means that when for example num=2, occ=2, max=3:
> atomic64_add_unless(&data->nexthops.num, occ, data->nexthops.max) won't fail when it should.
> 

ok, missed that in the description. I thought it was if the total would
equal or be greater than z.
diff mbox series

Patch

diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
index 45d8a7790bd5..3f48c0883225 100644
--- a/drivers/net/netdevsim/fib.c
+++ b/drivers/net/netdevsim/fib.c
@@ -31,7 +31,7 @@ 
 
 struct nsim_fib_entry {
 	u64 max;
-	u64 num;
+	atomic64_t num;
 };
 
 struct nsim_per_fib_data {
@@ -46,7 +46,7 @@  struct nsim_fib_data {
 	struct nsim_fib_entry nexthops;
 	struct rhashtable fib_rt_ht;
 	struct list_head fib_rt_list;
-	spinlock_t fib_lock;	/* Protects hashtable, list and accounting */
+	spinlock_t fib_lock;	/* Protects hashtable and list */
 	struct notifier_block nexthop_nb;
 	struct rhashtable nexthop_ht;
 	struct devlink *devlink;
@@ -128,7 +128,7 @@  u64 nsim_fib_get_val(struct nsim_fib_data *fib_data,
 		return 0;
 	}
 
-	return max ? entry->max : entry->num;
+	return max ? entry->max : atomic64_read(&entry->num);
 }
 
 static void nsim_fib_set_max(struct nsim_fib_data *fib_data,
@@ -165,14 +165,12 @@  static int nsim_fib_rule_account(struct nsim_fib_entry *entry, bool add,
 	int err = 0;
 
 	if (add) {
-		if (entry->num < entry->max) {
-			entry->num++;
-		} else {
+		if (!atomic64_add_unless(&entry->num, 1, entry->max)) {
 			err = -ENOSPC;
 			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported fib rule entries");
 		}
 	} else {
-		entry->num--;
+		atomic64_dec_if_positive(&entry->num);
 	}
 
 	return err;
@@ -202,14 +200,12 @@  static int nsim_fib_account(struct nsim_fib_entry *entry, bool add,
 	int err = 0;
 
 	if (add) {
-		if (entry->num < entry->max) {
-			entry->num++;
-		} else {
+		if (!atomic64_add_unless(&entry->num, 1, entry->max)) {
 			err = -ENOSPC;
 			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported fib entries");
 		}
 	} else {
-		entry->num--;
+		atomic64_dec_if_positive(&entry->num);
 	}
 
 	return err;
@@ -769,25 +765,22 @@  static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event,
 	struct fib_notifier_info *info = ptr;
 	int err = 0;
 
-	/* IPv6 routes can be added via RAs from softIRQ. */
-	spin_lock_bh(&data->fib_lock);
-
 	switch (event) {
 	case FIB_EVENT_RULE_ADD:
 	case FIB_EVENT_RULE_DEL:
 		err = nsim_fib_rule_event(data, info,
 					  event == FIB_EVENT_RULE_ADD);
 		break;
-
 	case FIB_EVENT_ENTRY_REPLACE:
 	case FIB_EVENT_ENTRY_APPEND:
 	case FIB_EVENT_ENTRY_DEL:
+		/* IPv6 routes can be added via RAs from softIRQ. */
+		spin_lock_bh(&data->fib_lock);
 		err = nsim_fib_event(data, info, event);
+		spin_unlock_bh(&data->fib_lock);
 		break;
 	}
 
-	spin_unlock_bh(&data->fib_lock);
-
 	return notifier_from_errno(err);
 }
 
@@ -847,8 +840,8 @@  static void nsim_fib_dump_inconsistent(struct notifier_block *nb)
 		nsim_fib_rt_free(fib_rt, data);
 	}
 
-	data->ipv4.rules.num = 0ULL;
-	data->ipv6.rules.num = 0ULL;
+	atomic64_set(&data->ipv4.rules.num, 0ULL);
+	atomic64_set(&data->ipv6.rules.num, 0ULL);
 }
 
 static struct nsim_nexthop *nsim_nexthop_create(struct nsim_fib_data *data,
@@ -889,22 +882,29 @@  static void nsim_nexthop_destroy(struct nsim_nexthop *nexthop)
 static int nsim_nexthop_account(struct nsim_fib_data *data, u64 occ,
 				bool add, struct netlink_ext_ack *extack)
 {
-	int err = 0;
+	int i, err = 0;
 
 	if (add) {
-		if (data->nexthops.num + occ <= data->nexthops.max) {
-			data->nexthops.num += occ;
-		} else {
-			err = -ENOSPC;
-			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
-		}
+		for (i = 0; i < occ; i++)
+			if (!atomic64_add_unless(&data->nexthops.num, 1,
+						 data->nexthops.max)) {
+				err = -ENOSPC;
+				NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
+				goto err_num_decrease;
+			}
 	} else {
-		if (WARN_ON(occ > data->nexthops.num))
+		if (WARN_ON(occ > atomic64_read(&data->nexthops.num)))
 			return -EINVAL;
-		data->nexthops.num -= occ;
+		atomic64_sub(occ, &data->nexthops.num);
 	}
 
 	return err;
+
+err_num_decrease:
+	for (i--; i >= 0; i--)
+		atomic64_dec(&data->nexthops.num);
+	return err;
+
 }
 
 static int nsim_nexthop_add(struct nsim_fib_data *data,