On Tue, Jul 2, 2024 at 6:09 PM Jakub Kicinski kuba@kernel.org wrote:
On Fri, 28 Jun 2024 00:32:40 +0000 Mina Almasry wrote:
if (binding->list.next)list_del(&binding->list);xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) {nit: s/bound_rxq_list/bound_rxqs/ ? it's not a list
if (rxq->mp_params.mp_priv == binding) {/* We hold the rtnl_lock while binding/unbinding* dma-buf, so we can't race with another thread that* is also modifying this value. However, the page_pool* may read this config while it's creating its* rx-queues. WRITE_ONCE() here to match the* READ_ONCE() in the page_pool.*/WRITE_ONCE(rxq->mp_params.mp_priv, NULL);Is this really sufficient in terms of locking? @binding is not RCU-protected and neither is the reader guaranteed to be in an RCU critical section. Actually the "reader" tries to take a ref and use this struct so it's not even a pure reader.
Let's add a lock or use one of the existing locks
Can we just use rtnl_lock() for this synchronization? It seems it's already locked everywhere that access mp_params.mp_priv, so the WRITE/READ_ONCE are actually superfluous. Both the dmabuf bind/unbind already lock rtnl_lock, and the only other place that access mp_params.mp_priv is page_pool_init(). I think it's reasonable to assume rtnl_lock is also held during page_pool_init, no? AFAICT it would be very weird for some code path to be reconfiguring the driver page_pools without holding rtnl_lock?
What I wanna do here is delete the incorrect comment, remove the READ/WRITE_ONCE, and maybe add a DEBUG_NET_WARN_ON(!rtnl_is_locked()) in mp_dmabuf_devmem_init() but probably that is too defensive.
Will apply the other comments, thanks.