>-----Original Message-----
>From: David Ahern <dsah...@gmail.com>
>Sent: Wednesday, January 27, 2021 6:33
>To: Ido Schimmel <ido...@idosch.org>; netdev@vger.kernel.org
>Cc: da...@davemloft.net; k...@kernel.org; Amit Cohen <amco...@nvidia.com>;
>Roopa Prabhu <ro...@nvidia.com>; Donald
>Sharp <sha...@nvidia.com>; Benjamin Poirier <bpoir...@nvidia.com>; mlxsw
><ml...@nvidia.com>; Ido Schimmel
><ido...@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,
>>
