On Tue, Jul 2, 2024 at 6:09 PM Jakub Kicinski <kuba@xxxxxxxxxx> 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.
--
Thanks,
Mina
