Re: [PATCH net-next V2 10/13] net: devmem: add netdev_has_dmabuf_binding() helper

[Dragos Tatulea <dtatulea@nvidia.com>]

On 11.11.26 22:46, Stanislav Fomichev wrote:
> On 11/10, Tariq Toukan wrote:
>> From: Dragos Tatulea <dtatulea@nvidia.com>
>>
>> Currently there is no way to check whether a dmabuf is bound on a TX
>> queue. RX side has netif_rxq_has_unreadable_mp() which gives a hint
>> of it.
>>
>> To help with that, add a helper to check whether a devmem dmabuf
>> binding may still be in use on a given netdev, optionally filtered by
>> the DMA device the dmabuf was mapped against.
>>
>> A binding can outlive the user's unbind: it can still be referenced by
>> in-flight TX skbs, whose DMA mappings are only valid for the device the
>> dmabuf was attached to. Reporting such a binding as gone would let a
>> driver swap the DMA device out from under those skbs. Bindings stay in
>> the bindings xarray until they are freed, so walk it and report every
>> binding of the netdev that is still mapped. Bindings that were already
>> unmapped are not reported.
>>
>> This API is necessary for the upcoming support of data direct in mlx5e.
>>
>> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
>> Cc: Bobby Eshleman <bobbyeshleman@gmail.com>
>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>> ---
>>  include/net/netmem.h | 24 ++++++++++++++++++++++++
>>  net/core/devmem.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/include/net/netmem.h b/include/net/netmem.h
>> index fb693bc1702a..35bb6fa0f719 100644
>> --- a/include/net/netmem.h
>> +++ b/include/net/netmem.h
>> @@ -397,16 +397,40 @@ static inline unsigned long netmem_get_dma_addr(netmem_ref netmem)
>>  	return netmem_to_nmdesc(netmem)->dma_addr;
>>  }
>>  
>> +struct net_device;
>> +
>>  #if defined(CONFIG_NET_DEVMEM)
>>  static inline bool net_is_devmem_iov(const struct net_iov *niov)
>>  {
>>  	return niov->type == NET_IOV_DMABUF;
>>  }
>> +
>> +/**
>> + * netdev_has_dmabuf_binding - is there a dmabuf binding for a
>> + * given @dev and @dma_dev
>> + *
>> + * @dev: netdev to check bindings
>> + * @dma_dev: dma device to check, when NULL it will not be checked
>> + *
>> + * A binding is reported for as long as it is mapped, which can be long
>> + * after the user unbound it: net_iovs of a TX binding can still be queued
>> + * in flight, with DMA mappings that are only valid for the device the dmabuf
>> + * was attached to.
>> + *
>> + * Return: true if a binding is found under @dev and possibly @dma_dev
>> + */
>> +bool netdev_has_dmabuf_binding(struct net_device *dev, struct device *dma_dev);
>>  #else
>>  static inline bool net_is_devmem_iov(const struct net_iov *niov)
>>  {
>>  	return false;
>>  }
>> +
>> +static inline bool netdev_has_dmabuf_binding(struct net_device *dev,
>> +					     struct device *dma_dev)
>> +{
>> +	return false;
>> +}
>>  #endif
>>  
>>  void net_iov_inc_ref(struct net_iov *niov);
>> diff --git a/net/core/devmem.c b/net/core/devmem.c
>> index 5c7d9640e9d1..dd3987bbd9f6 100644
>> --- a/net/core/devmem.c
>> +++ b/net/core/devmem.c
>> @@ -513,6 +513,49 @@ struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id)
>>  	return binding;
>>  }
>>  
>> +/* The attachment is cleared once the binding has been unmapped. */
>> +static bool
>> +net_devmem_binding_mapped_to(struct net_devmem_dmabuf_binding *binding,
>> +			     struct device *dma_dev)
>> +{
>> +	lockdep_assert_held(&binding->lock);
>> +
>> +	return binding->attachment &&
>> +	       (!dma_dev || binding->attachment->dev == dma_dev);
>> +}
>> +
>> +bool netdev_has_dmabuf_binding(struct net_device *dev, struct device *dma_dev)
>> +{
>> +	struct net_devmem_dmabuf_binding *binding;
>> +	unsigned long xa_idx;
>> +	bool found = false;
>> +
>> +	netdev_assert_locked(dev);
>> +
>> +	/* A binding stays in the xarray until it is freed. binding->dev is
>> +	 * only cleared under the instance lock of that netdev, and the free
>> +	 * takes it before erasing the binding, so a binding found under @dev
>> +	 * stays alive while the xarray lock is dropped.
>> +	 */
>> +	xa_lock(&net_devmem_dmabuf_bindings);
>> +	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
>> +		if (READ_ONCE(binding->dev) != dev)
>> +			continue;
>> +
>> +		xa_unlock(&net_devmem_dmabuf_bindings);
>> +		mutex_lock(&binding->lock);
>> +		found = net_devmem_binding_mapped_to(binding, dma_dev);
>> +		mutex_unlock(&binding->lock);
>> +		xa_lock(&net_devmem_dmabuf_bindings);
> 
> Sorry, my cache is already empty after c4466d2c3e77.. I wonder if it would
> be easier to change kfree(binding) to kfree_rcu(binding, ...) and then
> we can rcu_lock/xa_for_each here without xa_lock/mutex? All we care is our
> netdev bindings, and these are not being changed as long as we
> hold the instance lock (but the other ones are now concurrently freed,
> hence rcu so we can safely do READ_ONCE on the binding)..
> 
> And then the same applies to path 11?
> 
> (trying to keep all the complexity in net_devmem_dev_unregister)
Done. Will be part of v3. Code does look cleaner now for both patches.
Thanks for the suggestion!

Thanks,
Dragos