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

[Dragos Tatulea]

On 12.11.26 10:40, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The kernel-doc and commit message promise more than
>   netdev_has_dmabuf_binding() actually checks.
> 
> --
> 
> net: devmem: add netdev_has_dmabuf_binding() helper
> 
> This adds an exported helper, netdev_has_dmabuf_binding(). It walks the
> global devmem bindings xarray and reports whether a binding of a given
> netdev is still mapped, with an optional filter on the DMA device. The
> goal is to let drivers (data direct support in mlx5e) avoid swapping
> the DMA device while in-flight skbs may still use dmabuf mappings.
> 
>> diff --git a/include/net/netmem.h b/include/net/netmem.h
>> index fb693bc1702a4..35bb6fa0f7194 100644
>> --- a/include/net/netmem.h
>> +++ b/include/net/netmem.h
> 
> [ ... ]
> 
>> +/**
>> + * 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);
> 
> [ ... ]
> 
>> diff --git a/net/core/devmem.c b/net/core/devmem.c
>> index 5c7d9640e9d14..dd3987bbd9f6c 100644
>> --- a/net/core/devmem.c
>> +++ b/net/core/devmem.c
> 
> [ ... ]
> 
>> +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;
> 
> [Severity: Medium]
> Does this check match what the kernel-doc promises, "A binding is
> reported for as long as it is mapped"? The commit message also says the
> helper will "report every binding of the netdev that is still mapped".
> 
> For leased RX queues, binding->dev may not be the device that holds the
> mapping. netdev_nl_bind_rx_doit() passes the netdev the user named,
> which can be a virtual leasee such as netkit:
> 
> net/core/netdev-genl.c:netdev_nl_bind_rx_doit() {
>     ...
>     binding = net_devmem_bind_dmabuf(netdev, NULL, dma_dev, DMA_FROM_DEVICE,
>     ...
> }
> 
> The dma_dev, though, comes from the physical queue the RX queue is
> leased from:
> 
> net/core/netdev_queues.c:netdev_queue_get_dma_dev() {
>     ...
> 	hw_rxq = __netif_get_rx_queue(dev, idx)->lease;
> 
> 	netdev_lock(hw_rxq->dev);
> 	idx = get_netdev_rx_queue_index(hw_rxq);
> 	dma_dev = __netdev_queue_get_dma_dev(hw_rxq->dev, idx);
>     ...
> }
> 
> So binding->dev is the leasee, while binding->attachment->dev is the
> DMA device of the physical NIC. That also seems to disagree with the
> comment on struct net_devmem_dmabuf_binding::dev ("Physical NIC that
> does the actual DMA for this binding"), which this helper relies on.
> 
> On top of that, both teardown paths skip the unmap in the lease case
> but still clear binding->dev:
> 
> net/core/devmem.c:mp_dmabuf_devmem_uninstall() {
>     ...
> 			if (!rxq->lease)
> 				net_devmem_dmabuf_binding_unmap(binding);
> 			ASSERT_EXCLUSIVE_WRITER(binding->dev);
> 			WRITE_ONCE(binding->dev, NULL);
>     ...
> }
> 
> net/core/devmem.c:net_devmem_dev_unregister() {
>     ...
> 		if (!netif_is_queue_leasee(dev))
> 			net_devmem_dmabuf_binding_unmap(binding);
> 		xa_lock(&net_devmem_dmabuf_bindings);
> 		WRITE_ONCE(binding->dev, NULL);
>     ...
> }
> 
> After this, the binding stays in the xarray with attachment != NULL
> until in-flight net_iovs release it. The check above can no longer
> match it for any dev.
> 
> Could netdev_has_dmabuf_binding(phys_dev, phys_dma_dev) then return
> false while a binding is still mapped against phys_dma_dev?
> 
> The only caller in this series, set_pflag_data_direct() in mlx5e,
> already rejects installed RX memory providers and leased RX queues, so
> it may not hit this today. Since the helper is exported, would it make
> sense to either restrict the kernel-doc and commit message to TX
> bindings (or bindings whose dev is still set), or to also match
> bindings by attachment->dev against the DMA device(s) of @dev?

Ack. I will change the commit message and kernel-doc to mention leased
queues.

Thanks,
Dragos