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

-----Original Message-----
From: David Ahern <redacted>
Sent: Wednesday, January 27, 2021 6:33
To: Ido Schimmel <redacted>; <redacted>
Cc: davem@davemloft.net; kuba@kernel.org; Amit Cohen <redacted>; Roopa Prabhu <redacted>; Donald
Sharp [off-list ref]; Benjamin Poirier [off-list ref]; mlxsw [off-list ref]; Ido Schimmel
[off-list ref]
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:

quoted

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

quoted

+				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?

quoted

+	return err;
+
 }

 static int nsim_nexthop_add(struct nsim_fib_data *data,