On Wed, Apr 23, 2025 at 11:24 AM Michael S. Tsirkin <mst@xxxxxxxxxx> 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
