Re: [PATCH 4/9] drivers: Switch to kvfree_rcu() API

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Nov 24, 2021 at 12:03:03PM +0100, Uladzislau Rezki (Sony) wrote:
> Instead of invoking a synchronize_rcu() to free a pointer
> after a grace period we can directly make use of new API
> that does the same but in more efficient way.

It isn't entirely new, kfree_rcu() has been around for ages and any of
these call sites could have made use of it if they wanted. The
kvfree_rcu() just adds the twist of transparently allocating memory.

We really must ask in each case why the original author didn't use
kfree_rcu()..

>  drivers/block/drbd/drbd_nl.c                       | 9 +++------
>  drivers/block/drbd/drbd_receiver.c                 | 6 ++----
>  drivers/block/drbd/drbd_state.c                    | 3 +--
>  drivers/block/rnbd/rnbd-srv.c                      | 3 +--
>  drivers/crypto/nx/nx-common-pseries.c              | 3 +--
>  drivers/infiniband/hw/hfi1/sdma.c                  | 3 +--
>  drivers/ipack/carriers/tpci200.c                   | 3 +--
>  drivers/mfd/dln2.c                                 | 6 ++----
>  drivers/misc/vmw_vmci/vmci_context.c               | 6 ++----
>  drivers/misc/vmw_vmci/vmci_event.c                 | 3 +--
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    | 3 +--
>  drivers/net/ethernet/mellanox/mlx5/core/en/qos.c   | 3 +--
>  drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 3 +--
>  drivers/net/ethernet/mellanox/mlxsw/core.c         | 3 +--
>  drivers/scsi/device_handler/scsi_dh_alua.c         | 3 +--
>  drivers/scsi/device_handler/scsi_dh_rdac.c         | 3 +--
>  drivers/staging/fwserial/fwserial.c                | 3 +--

These all need to be split to single patches and ack'ed by experts.

> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
> index 44ccf8b4f4b2..28f4d84945fd 100644
> +++ b/drivers/block/drbd/drbd_nl.c
> @@ -1679,8 +1679,7 @@ int drbd_adm_disk_opts(struct sk_buff *skb, struct genl_info *info)
>  			drbd_send_sync_param(peer_device);
>  	}
>  
> -	synchronize_rcu();
> -	kfree(old_disk_conf);
> +	kvfree_rcu(old_disk_conf);
>  	kfree(old_plan);

For instance, this, how do you know that old_plan isn't also RCU
protected?

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index dde1cf51d0ab..0619cb94f0e0 100644
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -7190,8 +7190,7 @@ static void remove_one(struct pci_dev *pdev)
>  	}
>  	pci_release_regions(pdev);
>  	kfree(adapter->mbox_log);
> -	synchronize_rcu();
> -	kfree(adapter);
> +	kvfree_rcu(adapter);
>  }

And this, for instance, just looks crazy! There is only one RCU region
in this file and it is not protecting an adaptor pointer, it is
protecting a netdev. No idea what this is trying to do today even.

Each case needs to be audited to make sure the synchronize_rcu() is
only protecting the kfree and not other things as well. It is tricky
stuff.

I see you got an Ack for the infiniband peice, so feel free to send
that file to the linux-rdma list.

Jason