Re: [PATCH net-next v10 4/9] net: devmem: Implement TX path

[Mina Almasry <almasrymina@google.com>]

On Wed, Apr 23, 2025 at 11:24 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> some nits
>
> On Wed, Apr 23, 2025 at 03:11:11AM +0000, Mina Almasry wrote:
> > @@ -189,43 +200,44 @@ net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
> >       }
> >
> >       binding->dev = dev;
> > -
> > -     err = xa_alloc_cyclic(&net_devmem_dmabuf_bindings, &binding->id,
> > -                           binding, xa_limit_32b, &id_alloc_next,
> > -                           GFP_KERNEL);
> > -     if (err < 0)
> > -             goto err_free_binding;
> > -
> >       xa_init_flags(&binding->bound_rxqs, XA_FLAGS_ALLOC);
> > -
> >       refcount_set(&binding->ref, 1);
> > -
> >       binding->dmabuf = dmabuf;
> >
>
> given you keep iterating, don't tweak whitespace in the same patch-
> will make the review a tiny bit easier.
>

Sure.

>
> >       binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent);
> >       if (IS_ERR(binding->attachment)) {
> >               err = PTR_ERR(binding->attachment);
> >               NL_SET_ERR_MSG(extack, "Failed to bind dmabuf to device");
> > -             goto err_free_id;
> > +             goto err_free_binding;
> >       }
> >
> >       binding->sgt = dma_buf_map_attachment_unlocked(binding->attachment,
> > -                                                    DMA_FROM_DEVICE);
> > +                                                    direction);
> >       if (IS_ERR(binding->sgt)) {
> >               err = PTR_ERR(binding->sgt);
> >               NL_SET_ERR_MSG(extack, "Failed to map dmabuf attachment");
> >               goto err_detach;
> >       }
> >
> > +     if (direction == DMA_TO_DEVICE) {
> > +             binding->tx_vec = kvmalloc_array(dmabuf->size / PAGE_SIZE,
> > +                                              sizeof(struct net_iov *),
> > +                                              GFP_KERNEL);
> > +             if (!binding->tx_vec) {
> > +                     err = -ENOMEM;
> > +                     goto err_unmap;
> > +             }
> > +     }
> > +
> >       /* For simplicity we expect to make PAGE_SIZE allocations, but the
> >        * binding can be much more flexible than that. We may be able to
> >        * allocate MTU sized chunks here. Leave that for future work...
> >        */
> > -     binding->chunk_pool =
> > -             gen_pool_create(PAGE_SHIFT, dev_to_node(&dev->dev));
> > +     binding->chunk_pool = gen_pool_create(PAGE_SHIFT,
> > +                                           dev_to_node(&dev->dev));
> >       if (!binding->chunk_pool) {
> >               err = -ENOMEM;
> > -             goto err_unmap;
> > +             goto err_tx_vec;
> >       }
> >
> >       virtual = 0;
> > @@ -270,24 +282,34 @@ net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
> >                       niov->owner = &owner->area;
> >                       page_pool_set_dma_addr_netmem(net_iov_to_netmem(niov),
> >                                                     net_devmem_get_dma_addr(niov));
> > +                     if (direction == DMA_TO_DEVICE)
> > +                             binding->tx_vec[owner->area.base_virtual / PAGE_SIZE + i] = niov;
> >               }
> >
> >               virtual += len;
> >       }
> >
> > +     err = xa_alloc_cyclic(&net_devmem_dmabuf_bindings, &binding->id,
> > +                           binding, xa_limit_32b, &id_alloc_next,
> > +                           GFP_KERNEL);
> > +     if (err < 0)
> > +             goto err_free_id;
> > +
> >       return binding;
> >
> > +err_free_id:
> > +     xa_erase(&net_devmem_dmabuf_bindings, binding->id);
> >  err_free_chunks:
> >       gen_pool_for_each_chunk(binding->chunk_pool,
> >                               net_devmem_dmabuf_free_chunk_owner, NULL);
> >       gen_pool_destroy(binding->chunk_pool);
> > +err_tx_vec:
> > +     kvfree(binding->tx_vec);
> >  err_unmap:
> >       dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
> >                                         DMA_FROM_DEVICE);
> >  err_detach:
> >       dma_buf_detach(dmabuf, binding->attachment);
> > -err_free_id:
> > -     xa_erase(&net_devmem_dmabuf_bindings, binding->id);
> >  err_free_binding:
> >       kfree(binding);
> >  err_put_dmabuf:
> > @@ -295,6 +317,21 @@ net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
> >       return ERR_PTR(err);
> >  }
> >
> > +struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id)
> > +{
> > +     struct net_devmem_dmabuf_binding *binding;
> > +
> > +     rcu_read_lock();
> > +     binding = xa_load(&net_devmem_dmabuf_bindings, id);
> > +     if (binding) {
> > +             if (!net_devmem_dmabuf_binding_get(binding))
> > +                     binding = NULL;
> > +     }
> > +     rcu_read_unlock();
> > +
> > +     return binding;
> > +}
> > +
> >  void net_devmem_get_net_iov(struct net_iov *niov)
> >  {
> >       net_devmem_dmabuf_binding_get(net_devmem_iov_binding(niov));
> > @@ -305,6 +342,53 @@ void net_devmem_put_net_iov(struct net_iov *niov)
> >       net_devmem_dmabuf_binding_put(net_devmem_iov_binding(niov));
> >  }
> >
> > +struct net_devmem_dmabuf_binding *net_devmem_get_binding(struct sock *sk,
> > +                                                      unsigned int dmabuf_id)
> > +{
> > +     struct net_devmem_dmabuf_binding *binding;
> > +     struct dst_entry *dst = __sk_dst_get(sk);
> > +     int err = 0;
> > +
> > +     binding = net_devmem_lookup_dmabuf(dmabuf_id);
>
> why not initialize binding together with the declaration?
>

I find it stylistically much better to error check this right after
it's assigned.

> > +     if (!binding || !binding->tx_vec) {
> > +             err = -EINVAL;
> > +             goto out_err;
> > +     }
> > +
> > +     /* The dma-addrs in this binding are only reachable to the corresponding
> > +      * net_device.
> > +      */
> > +     if (!dst || !dst->dev || dst->dev->ifindex != binding->dev->ifindex) {
> > +             err = -ENODEV;
> > +             goto out_err;
> > +     }
> > +
> > +     return binding;
> > +
> > +out_err:
> > +     if (binding)
> > +             net_devmem_dmabuf_binding_put(binding);
> > +
> > +     return ERR_PTR(err);
> > +}
> > +
> > +struct net_iov *
> > +net_devmem_get_niov_at(struct net_devmem_dmabuf_binding *binding,
> > +                    size_t virt_addr, size_t *off, size_t *size)
> > +{
> > +     size_t idx;
> > +
> > +     if (virt_addr >= binding->dmabuf->size)
> > +             return NULL;
> > +
> > +     idx = virt_addr / PAGE_SIZE;
>
> init this at where it's declared?
> or where it's used.
>

I think probably a local variable isn't warranted. Will remove.

-- 
Thanks,
Mina