| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHS8izNRk1MOs-R1RmWMQ5zMBOo70YpTzOdt8=OrrjrD7JQTfA@mail.gmail.com>
Date: Wed, 23 Apr 2025 16:49:30 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, io-uring@...r.kernel.org,
virtualization@...ts.linux.dev, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org, Donald Hunter <donald.hunter@...il.com>,
Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Jonathan Corbet <corbet@....net>, Andrew Lunn <andrew+netdev@...n.ch>,
Jeroen de Borst <jeroendb@...gle.com>, Harshitha Ramamurthy <hramamurthy@...gle.com>,
Kuniyuki Iwashima <kuniyu@...zon.com>, Willem de Bruijn <willemb@...gle.com>, Jens Axboe <axboe@...nel.dk>,
Pavel Begunkov <asml.silence@...il.com>, David Ahern <dsahern@...nel.org>,
Neal Cardwell <ncardwell@...gle.com>, Stefan Hajnoczi <stefanha@...hat.com>,
Stefano Garzarella <sgarzare@...hat.com>, Jason Wang <jasowang@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez <eperezma@...hat.com>,
Shuah Khan <shuah@...nel.org>, sdf@...ichev.me, dw@...idwei.uk,
Jamal Hadi Salim <jhs@...atatu.com>, Victor Nogueira <victor@...atatu.com>,
Pedro Tammela <pctammela@...atatu.com>, Samiullah Khawaja <skhawaja@...gle.com>,
Kaiyuan Zhang <kaiyuanz@...gle.com>
Subject: Re: [PATCH net-next v10 4/9] net: devmem: Implement TX path
On Wed, Apr 23, 2025 at 11:24 AM Michael S. Tsirkin <mst@...hat.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
Powered by blists - more mailing lists