| Message ID | 20230810015751.3297321-3-almasrymina@google.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Device Memory TCP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On Wed, Aug 09, 2023 at 06:57:38PM -0700, Mina Almasry wrote: > Add a netdev_dmabuf_binding struct which represents the > dma-buf-to-netdevice binding. The netlink API will bind the dma-buf to > an rx queue on the netdevice. On the binding, the dma_buf_attach > & dma_buf_map_attachment will occur. The entries in the sg_table from > mapping will be inserted into a genpool to make it ready > for allocation. > > The chunks in the genpool are owned by a dmabuf_chunk_owner struct which > holds the dma-buf offset of the base of the chunk and the dma_addr of > the chunk. Both are needed to use allocations that come from this chunk. > > We create a new type that represents an allocation from the genpool: > page_pool_iov. We setup the page_pool_iov allocation size in the > genpool to PAGE_SIZE for simplicity: to match the PAGE_SIZE normally > allocated by the page pool and given to the drivers. > > The user can unbind the dmabuf from the netdevice by closing the netlink > socket that established the binding. We do this so that the binding is > automatically unbound even if the userspace process crashes. > > The binding and unbinding leaves an indicator in struct netdev_rx_queue > that the given queue is bound, but the binding doesn't take effect until > the driver actually reconfigures its queues, and re-initializes its page > pool. This issue/weirdness is highlighted in the memory provider > proposal[1], and I'm hoping that some generic solution for all > memory providers will be discussed; this patch doesn't address that > weirdness again. > > The netdev_dmabuf_binding struct is refcounted, and releases its > resources only when all the refs are released. > > [1] https://lore.kernel.org/netdev/20230707183935.997267-1-kuba@kernel.org/ > > Signed-off-by: Willem de Bruijn <willemb@google.com> > Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com> > > Signed-off-by: Mina Almasry <almasrymina@google.com> > --- > include/linux/netdevice.h | 57 ++++++++++++ > include/net/page_pool.h | 27 ++++++ > net/core/dev.c | 178 ++++++++++++++++++++++++++++++++++++++ > net/core/netdev-genl.c | 101 ++++++++++++++++++++- > 4 files changed, 361 insertions(+), 2 deletions(-) <...> > +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding); > + > +static inline void > +netdev_devmem_binding_get(struct netdev_dmabuf_binding *binding) > +{ > + refcount_inc(&binding->ref); > +} > + > +static inline void > +netdev_devmem_binding_put(struct netdev_dmabuf_binding *binding) > +{ > + if (!refcount_dec_and_test(&binding->ref)) > + return; > + > + __netdev_devmem_binding_free(binding); > +} Not a big deal, but it looks like reimplemented version of kref_get/kref_put to me. Thanks
On 8/9/23 7:57 PM, Mina Almasry wrote: > diff --git a/net/core/dev.c b/net/core/dev.c > index 8e7d0cb540cd..02a25ccf771a 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -151,6 +151,8 @@ > #include <linux/pm_runtime.h> > #include <linux/prandom.h> > #include <linux/once_lite.h> > +#include <linux/genalloc.h> > +#include <linux/dma-buf.h> > > #include "dev.h" > #include "net-sysfs.h" > @@ -2037,6 +2039,182 @@ static int call_netdevice_notifiers_mtu(unsigned long val, > return call_netdevice_notifiers_info(val, &info.info); > } > > +/* Device memory support */ > + > +static void netdev_devmem_free_chunk_owner(struct gen_pool *genpool, > + struct gen_pool_chunk *chunk, > + void *not_used) > +{ > + struct dmabuf_genpool_chunk_owner *owner = chunk->owner; > + > + kvfree(owner->ppiovs); > + kfree(owner); > +} > + > +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding) > +{ > + size_t size, avail; > + > + gen_pool_for_each_chunk(binding->chunk_pool, > + netdev_devmem_free_chunk_owner, NULL); > + > + size = gen_pool_size(binding->chunk_pool); > + avail = gen_pool_avail(binding->chunk_pool); > + > + if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu", > + size, avail)) > + gen_pool_destroy(binding->chunk_pool); > + > + dma_buf_unmap_attachment(binding->attachment, binding->sgt, > + DMA_BIDIRECTIONAL); > + dma_buf_detach(binding->dmabuf, binding->attachment); > + dma_buf_put(binding->dmabuf); > + kfree(binding); > +} > + > +void netdev_unbind_dmabuf_to_queue(struct netdev_dmabuf_binding *binding) > +{ > + struct netdev_rx_queue *rxq; > + unsigned long xa_idx; > + > + list_del_rcu(&binding->list); > + > + xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) > + if (rxq->binding == binding) > + rxq->binding = NULL; > + > + netdev_devmem_binding_put(binding); This does a put on the binding but it does not notify the driver that that the dmabuf references need to be flushed from the rx queue. Also, what about the device getting deleted - e.g., the driver is removed? > +} > + > +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int dmabuf_fd, > + u32 rxq_idx, struct netdev_dmabuf_binding **out) > +{ > + struct netdev_dmabuf_binding *binding; > + struct netdev_rx_queue *rxq; > + struct scatterlist *sg; > + struct dma_buf *dmabuf; > + unsigned int sg_idx, i; > + unsigned long virtual; > + u32 xa_idx; > + int err; > + > + rxq = __netif_get_rx_queue(dev, rxq_idx); > + > + if (rxq->binding) > + return -EEXIST; So this proposal is limiting a binding to a single dmabuf at a time? Is this just for the RFC? Also, this suggests that the Rx queue is unique to the flow. I do not recall a netdev API to create H/W queues on the fly (only a passing comment from Kuba), so how is the H/W queue (or queue set since a completion queue is needed as well) created for the flow? And in turn if it is unique to the flow, what deletes the queue if an app does not do a proper cleanup? If the queue sticks around, the dmabuf references stick around. Also, if this is an app specific h/w queue, flow steering is not mentioned in this RFC. > + > + dmabuf = dma_buf_get(dmabuf_fd); > + if (IS_ERR_OR_NULL(dmabuf)) > + return -EBADFD; > + > + binding = kzalloc_node(sizeof(*binding), GFP_KERNEL, > + dev_to_node(&dev->dev)); > + if (!binding) { > + err = -ENOMEM; > + goto err_put_dmabuf; > + } > + > + xa_init_flags(&binding->bound_rxq_list, XA_FLAGS_ALLOC); > + > + refcount_set(&binding->ref, 1); > + > + binding->dmabuf = dmabuf; > + > + binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent); > + if (IS_ERR(binding->attachment)) { > + err = PTR_ERR(binding->attachment); > + goto err_free_binding; > + } > + > + binding->sgt = dma_buf_map_attachment(binding->attachment, > + DMA_BIDIRECTIONAL); > + if (IS_ERR(binding->sgt)) { > + err = PTR_ERR(binding->sgt); > + goto err_detach; > + } > + > + /* 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)); > + if (!binding->chunk_pool) { > + err = -ENOMEM; > + goto err_unmap; > + } > + > + virtual = 0; > + for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) { > + dma_addr_t dma_addr = sg_dma_address(sg); > + struct dmabuf_genpool_chunk_owner *owner; > + size_t len = sg_dma_len(sg); > + struct page_pool_iov *ppiov; > + > + owner = kzalloc_node(sizeof(*owner), GFP_KERNEL, > + dev_to_node(&dev->dev)); > + owner->base_virtual = virtual; > + owner->base_dma_addr = dma_addr; > + owner->num_ppiovs = len / PAGE_SIZE; > + owner->binding = binding; > + > + err = gen_pool_add_owner(binding->chunk_pool, dma_addr, > + dma_addr, len, dev_to_node(&dev->dev), > + owner); > + if (err) { > + err = -EINVAL; > + goto err_free_chunks; > + } > + > + owner->ppiovs = kvmalloc_array(owner->num_ppiovs, > + sizeof(*owner->ppiovs), > + GFP_KERNEL); > + if (!owner->ppiovs) { > + err = -ENOMEM; > + goto err_free_chunks; > + } > + > + for (i = 0; i < owner->num_ppiovs; i++) { > + ppiov = &owner->ppiovs[i]; > + ppiov->owner = owner; > + refcount_set(&ppiov->refcount, 1); > + } > + > + dma_addr += len; > + virtual += len; > + } > + > + /* TODO: need to be able to bind to multiple rx queues on the same > + * netdevice. The code should already be able to handle that with > + * minimal changes, but the netlink API currently allows for 1 rx > + * queue. > + */ > + err = xa_alloc(&binding->bound_rxq_list, &xa_idx, rxq, xa_limit_32b, > + GFP_KERNEL); > + if (err) > + goto err_free_chunks; > + > + rxq->binding = binding; > + *out = binding; > + > + return 0; > + > +err_free_chunks: > + gen_pool_for_each_chunk(binding->chunk_pool, > + netdev_devmem_free_chunk_owner, NULL); > + gen_pool_destroy(binding->chunk_pool); > +err_unmap: > + dma_buf_unmap_attachment(binding->attachment, binding->sgt, > + DMA_BIDIRECTIONAL); > +err_detach: > + dma_buf_detach(dmabuf, binding->attachment); > +err_free_binding: > + kfree(binding); > +err_put_dmabuf: > + dma_buf_put(dmabuf); > + return err; > +} > + > #ifdef CONFIG_NET_INGRESS > static DEFINE_STATIC_KEY_FALSE(ingress_needed_key); > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > index bf7324dd6c36..288ed0112995 100644 > --- a/net/core/netdev-genl.c > +++ b/net/core/netdev-genl.c> @@ -167,10 +231,37 @@ static int netdev_genl_netdevice_event(struct notifier_block *nb, > return NOTIFY_OK; > } > > +static int netdev_netlink_notify(struct notifier_block *nb, unsigned long state, > + void *_notify) > +{ > + struct netlink_notify *notify = _notify; > + struct netdev_dmabuf_binding *rbinding; > + > + if (state != NETLINK_URELEASE || notify->protocol != NETLINK_GENERIC) > + return NOTIFY_DONE; > + > + rcu_read_lock(); > + > + list_for_each_entry_rcu(rbinding, &netdev_rbinding_list, list) { > + if (rbinding->owner_nlportid == notify->portid) { > + netdev_unbind_dmabuf_to_queue(rbinding); This ties the removal of a dmabuf to the close of the netlink socket as suggested in the previous round of comments. What happens if the process closes the dmabuf fd? Is the outstanding dev binding sufficient to keep the allocation / use in place?
On Sun, Aug 13, 2023 at 6:10 PM David Ahern <dsahern@kernel.org> wrote: > > On 8/9/23 7:57 PM, Mina Almasry wrote: > > diff --git a/net/core/dev.c b/net/core/dev.c > > index 8e7d0cb540cd..02a25ccf771a 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -151,6 +151,8 @@ > > #include <linux/pm_runtime.h> > > #include <linux/prandom.h> > > #include <linux/once_lite.h> > > +#include <linux/genalloc.h> > > +#include <linux/dma-buf.h> > > > > #include "dev.h" > > #include "net-sysfs.h" > > @@ -2037,6 +2039,182 @@ static int call_netdevice_notifiers_mtu(unsigned long val, > > return call_netdevice_notifiers_info(val, &info.info); > > } > > > > +/* Device memory support */ > > + > > +static void netdev_devmem_free_chunk_owner(struct gen_pool *genpool, > > + struct gen_pool_chunk *chunk, > > + void *not_used) > > +{ > > + struct dmabuf_genpool_chunk_owner *owner = chunk->owner; > > + > > + kvfree(owner->ppiovs); > > + kfree(owner); > > +} > > + > > +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding) > > +{ > > + size_t size, avail; > > + > > + gen_pool_for_each_chunk(binding->chunk_pool, > > + netdev_devmem_free_chunk_owner, NULL); > > + > > + size = gen_pool_size(binding->chunk_pool); > > + avail = gen_pool_avail(binding->chunk_pool); > > + > > + if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu", > > + size, avail)) > > + gen_pool_destroy(binding->chunk_pool); > > + > > + dma_buf_unmap_attachment(binding->attachment, binding->sgt, > > + DMA_BIDIRECTIONAL); > > + dma_buf_detach(binding->dmabuf, binding->attachment); > > + dma_buf_put(binding->dmabuf); > > + kfree(binding); > > +} > > + > > +void netdev_unbind_dmabuf_to_queue(struct netdev_dmabuf_binding *binding) > > +{ > > + struct netdev_rx_queue *rxq; > > + unsigned long xa_idx; > > + > > + list_del_rcu(&binding->list); > > + > > + xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) > > + if (rxq->binding == binding) > > + rxq->binding = NULL; > > + > > + netdev_devmem_binding_put(binding); > > This does a put on the binding but it does not notify the driver that > that the dmabuf references need to be flushed from the rx queue. > Correct, FWIW this is called out in the commit message of this patch, and is a general issue with all memory providers and not really specific to the memory provider added for devmem TCP. Jakub described the issue in the cover letter of the memory provider proposal: https://lore.kernel.org/netdev/20230707183935.997267-1-kuba@kernel.org/ For now the selftest triggers a driver reset after bind & unbind for the configuration to take effect. I think the right thing to do is a generic solution should be applied to the general memory provider proposal and devmem TCP will follow that. One way to resolve this could be to trigger ethtool_ops->reset() call on any memory provider configuration, which would recreate the queues as part of the reset. Or adding a new API (ethtool op or otherwise) which will only recreate the queues (or a specific queue). > Also, what about the device getting deleted - e.g., the driver is removed? > Good point, I don't think I'm handling that correctly. I'm not sure what the solution is at the moment. It probably is not right for the bind to do a netdev_hold(), because it doesn't make much sense for the dma-buf binding to keep the netdev alive, I think. So probably the netdev freeing needs to unbind from the dma-buf, and the netlink unbind needs to not duplicate the unbind. Should be simple to implement I think. Thanks for catching. > > +} > > + > > +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int dmabuf_fd, > > + u32 rxq_idx, struct netdev_dmabuf_binding **out) > > +{ > > + struct netdev_dmabuf_binding *binding; > > + struct netdev_rx_queue *rxq; > > + struct scatterlist *sg; > > + struct dma_buf *dmabuf; > > + unsigned int sg_idx, i; > > + unsigned long virtual; > > + u32 xa_idx; > > + int err; > > + > > + rxq = __netif_get_rx_queue(dev, rxq_idx); > > + > > + if (rxq->binding) > > + return -EEXIST; > > So this proposal is limiting a binding to a single dmabuf at a time? Is > this just for the RFC? > I'm only allowing 1 rx queue to be bound to 1 dma-buf, and that is a permanent restriction, I think. It would be amazing if we could bind multiple dma-bufs to the same rx queue and the driver could somehow know which dma-buf this packet is destined for. Unfortunately I don't think drivers can do this without fancy parsing of incoming traffic, and devmem TCP is possible without such driver support - as long as we restrict 1 dma-buf per queue. > Also, this suggests that the Rx queue is unique to the flow. I do not > recall a netdev API to create H/W queues on the fly (only a passing > comment from Kuba), so how is the H/W queue (or queue set since a > completion queue is needed as well) created for the flow? And in turn if > it is unique to the flow, what deletes the queue if an app does not do a > proper cleanup? If the queue sticks around, the dmabuf references stick > around. > An RX queue is unique to an application & its dma-buf, not a single flow. It is possible for the application to bind its dma-buf to an rx queue, then steer multiple flows to that rx queue, and receive incoming packets from these multiple flows onto its dma-buf. Not implemented in this POC RFC, but will be implemented in the next version: it should also be possible for the application to bind its dma-buf to multiple rx queues, and steer its flows to one of these rx queues, and receive incoming packets on the dma-buf. I'm currently not thinking along the lines of creating a new H/W queue for each new devmem flow. Instead, an existing queue gets re-purposed for device memory TCP by binding it to a dma-buf and configuring flow steering & RSS to steer this dma-buf owner's flows onto this rx queue. We could go in the direction of creating new H/W queues for each dma-buf binding if you think there is some upside. Off the top of my head, I think the current model fits in better with the general memory-provider plans which configure existing queues rather than create new ones. > Also, if this is an app specific h/w queue, flow steering is not > mentioned in this RFC. > Technically it's not an app-specific h/w queue. In theory it's also possible for multiple applications running under the same user to share a single dma-buf which is bound to any number of rx queues, and for all these applications to receive incoming packets on the shared dma-buf simultaneously. Flow steering is mentioned as a dependency in the cover letter, but I've largely neglected to elaborate on how the use case works end-to-end with userspace flow steering & RSS configuration, largely because the APIs are flexible to handle many different use cases. Sorry about that, I'll add a section regarding that in the next iteration. > > + > > + dmabuf = dma_buf_get(dmabuf_fd); > > + if (IS_ERR_OR_NULL(dmabuf)) > > + return -EBADFD; > > + > > + binding = kzalloc_node(sizeof(*binding), GFP_KERNEL, > > + dev_to_node(&dev->dev)); > > + if (!binding) { > > + err = -ENOMEM; > > + goto err_put_dmabuf; > > + } > > + > > + xa_init_flags(&binding->bound_rxq_list, XA_FLAGS_ALLOC); > > + > > + refcount_set(&binding->ref, 1); > > + > > + binding->dmabuf = dmabuf; > > + > > + binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent); > > + if (IS_ERR(binding->attachment)) { > > + err = PTR_ERR(binding->attachment); > > + goto err_free_binding; > > + } > > + > > + binding->sgt = dma_buf_map_attachment(binding->attachment, > > + DMA_BIDIRECTIONAL); > > + if (IS_ERR(binding->sgt)) { > > + err = PTR_ERR(binding->sgt); > > + goto err_detach; > > + } > > + > > + /* 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)); > > + if (!binding->chunk_pool) { > > + err = -ENOMEM; > > + goto err_unmap; > > + } > > + > > + virtual = 0; > > + for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) { > > + dma_addr_t dma_addr = sg_dma_address(sg); > > + struct dmabuf_genpool_chunk_owner *owner; > > + size_t len = sg_dma_len(sg); > > + struct page_pool_iov *ppiov; > > + > > + owner = kzalloc_node(sizeof(*owner), GFP_KERNEL, > > + dev_to_node(&dev->dev)); > > + owner->base_virtual = virtual; > > + owner->base_dma_addr = dma_addr; > > + owner->num_ppiovs = len / PAGE_SIZE; > > + owner->binding = binding; > > + > > + err = gen_pool_add_owner(binding->chunk_pool, dma_addr, > > + dma_addr, len, dev_to_node(&dev->dev), > > + owner); > > + if (err) { > > + err = -EINVAL; > > + goto err_free_chunks; > > + } > > + > > + owner->ppiovs = kvmalloc_array(owner->num_ppiovs, > > + sizeof(*owner->ppiovs), > > + GFP_KERNEL); > > + if (!owner->ppiovs) { > > + err = -ENOMEM; > > + goto err_free_chunks; > > + } > > + > > + for (i = 0; i < owner->num_ppiovs; i++) { > > + ppiov = &owner->ppiovs[i]; > > + ppiov->owner = owner; > > + refcount_set(&ppiov->refcount, 1); > > + } > > + > > + dma_addr += len; > > + virtual += len; > > + } > > + > > + /* TODO: need to be able to bind to multiple rx queues on the same > > + * netdevice. The code should already be able to handle that with > > + * minimal changes, but the netlink API currently allows for 1 rx > > + * queue. > > + */ > > + err = xa_alloc(&binding->bound_rxq_list, &xa_idx, rxq, xa_limit_32b, > > + GFP_KERNEL); > > + if (err) > > + goto err_free_chunks; > > + > > + rxq->binding = binding; > > + *out = binding; > > + > > + return 0; > > + > > +err_free_chunks: > > + gen_pool_for_each_chunk(binding->chunk_pool, > > + netdev_devmem_free_chunk_owner, NULL); > > + gen_pool_destroy(binding->chunk_pool); > > +err_unmap: > > + dma_buf_unmap_attachment(binding->attachment, binding->sgt, > > + DMA_BIDIRECTIONAL); > > +err_detach: > > + dma_buf_detach(dmabuf, binding->attachment); > > +err_free_binding: > > + kfree(binding); > > +err_put_dmabuf: > > + dma_buf_put(dmabuf); > > + return err; > > +} > > + > > #ifdef CONFIG_NET_INGRESS > > static DEFINE_STATIC_KEY_FALSE(ingress_needed_key); > > > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > > index bf7324dd6c36..288ed0112995 100644 > > --- a/net/core/netdev-genl.c > > +++ b/net/core/netdev-genl.c> @@ -167,10 +231,37 @@ static int netdev_genl_netdevice_event(struct > notifier_block *nb, > > return NOTIFY_OK; > > } > > > > +static int netdev_netlink_notify(struct notifier_block *nb, unsigned long state, > > + void *_notify) > > +{ > > + struct netlink_notify *notify = _notify; > > + struct netdev_dmabuf_binding *rbinding; > > + > > + if (state != NETLINK_URELEASE || notify->protocol != NETLINK_GENERIC) > > + return NOTIFY_DONE; > > + > > + rcu_read_lock(); > > + > > + list_for_each_entry_rcu(rbinding, &netdev_rbinding_list, list) { > > + if (rbinding->owner_nlportid == notify->portid) { > > + netdev_unbind_dmabuf_to_queue(rbinding); > > This ties the removal of a dmabuf to the close of the netlink socket as > suggested in the previous round of comments. What happens if the process > closes the dmabuf fd? Is the outstanding dev binding sufficient to keep > the allocation / use in place? > Correct, the outstanding dev binding keeps the dma-buf & its attachment in place until the driver no longer needs the binding and drops the references.
On Sun, 13 Aug 2023 19:10:35 -0600 David Ahern wrote: > Also, this suggests that the Rx queue is unique to the flow. I do not > recall a netdev API to create H/W queues on the fly (only a passing > comment from Kuba), so how is the H/W queue (or queue set since a > completion queue is needed as well) created for the flow? > And in turn if it is unique to the flow, what deletes the queue if > an app does not do a proper cleanup? If the queue sticks around, > the dmabuf references stick around. Let's start sketching out the design for queue config. Without sliding into scope creep, hopefully. Step one - I think we can decompose the problem into: A) flow steering B) object lifetime and permissions C) queue configuration (incl. potentially creating / destroying queues) These come together into use scenarios like: #1 - partitioning for containers - when high perf containers share a machine each should get an RSS context on the physical NIC to have predictable traffic<>CPU placement, they may also have different preferences on how the queues are configured, maybe XDP, too? #2 - fancy page pools within the host (e.g. huge pages) #3 - very fancy page pools not within the host (Mina's work) #4 - XDP redirect target (allowing XDP_REDIRECT without installing XDP on the target) #5 - busy polling - admittedly a bit theoretical, I don't know of anyone busy polling in real life, but one of the problems today is that setting it up requires scraping random bits of info from sysfs and a lot of hoping. Flow steering (A) is there today, to a sufficient extent, I think, so we can defer on that. Sooner or later we should probably figure out if we want to continue down the unruly path of TC offloads or just give up and beef up ethtool. I don't have a good sense of what a good model for cleanup and permissions is (B). All I know is that if we need to tie things to processes netlink can do it, and we shouldn't have to create our own FS and special file descriptors... And then there's (C) which is the main part to talk about. The first step IMHO is to straighten out the configuration process. Currently we do: user -> thin ethtool API --------------------> driver netdev core <---' By "straighten" I mean more of a: user -> thin ethtool API ---> netdev core ---> driver flow. This means core maintains the full expected configuration, queue count and their parameters and driver creates those queues as instructed. I'd imagine we'd need 4 basic ops: - queue_mem_alloc(dev, cfg) -> queue_mem - queue_mem_free(dev, cfg, queue_mem) - queue_start(dev, queue info, cfg, queue_mem) -> errno - queue_stop(dev, queue info, cfg) The mem_alloc/mem_free takes care of the commonly missed requirement to not take the datapath down until resources are allocated for new config. Core then sets all the queues up after ndo_open, and tears down before ndo_stop. In case of an ethtool -L / -G call or enabling / disabling XDP core can handle the entire reconfiguration dance. The cfg object needs to contain all queue configuration, including the page pool parameters. If we have an abstract model of the configuration in the core we can modify it much more easily, I hope. I mean - the configuration will be somewhat detached from what's instantiated in the drivers. I'd prefer to go as far as we can without introducing a driver callback to "check if it can support a config change", and try to rely on (static) capabilities instead. This allows more of the validation to happen in the core and also lends itself naturally to exporting the capabilities to the user. Checking the use cases: #1 - partitioning for containers - storing the cfg in the core gives us a neat ability to allow users to set the configuration on RSS context #2, #3 - page pools - we can make page_pool_create take cfg and read whatever params we want from there, memory provider, descriptor count, recycling ring size etc. Also for header-data-split we may want different settings per queue so again cfg comes in handy #4 - XDP redirect target - we should spawn XDP TX queues independently from the XDP configuration That's all I have thought up in terms of direction. Does that make sense? What are the main gaps? Other proposals?
Jakub Kicinski wrote: > On Sun, 13 Aug 2023 19:10:35 -0600 David Ahern wrote: > > Also, this suggests that the Rx queue is unique to the flow. I do not > > recall a netdev API to create H/W queues on the fly (only a passing > > comment from Kuba), so how is the H/W queue (or queue set since a > > completion queue is needed as well) created for the flow? > > And in turn if it is unique to the flow, what deletes the queue if > > an app does not do a proper cleanup? If the queue sticks around, > > the dmabuf references stick around. > > Let's start sketching out the design for queue config. > Without sliding into scope creep, hopefully. > > Step one - I think we can decompose the problem into: > A) flow steering > B) object lifetime and permissions > C) queue configuration (incl. potentially creating / destroying queues) > > These come together into use scenarios like: > #1 - partitioning for containers - when high perf containers share > a machine each should get an RSS context on the physical NIC > to have predictable traffic<>CPU placement, they may also have > different preferences on how the queues are configured, maybe > XDP, too? > #2 - fancy page pools within the host (e.g. huge pages) > #3 - very fancy page pools not within the host (Mina's work) > #4 - XDP redirect target (allowing XDP_REDIRECT without installing XDP > on the target) > #5 - busy polling - admittedly a bit theoretical, I don't know of > anyone busy polling in real life, but one of the problems today > is that setting it up requires scraping random bits of info from > sysfs and a lot of hoping. > > Flow steering (A) is there today, to a sufficient extent, I think, > so we can defer on that. Sooner or later we should probably figure > out if we want to continue down the unruly path of TC offloads or > just give up and beef up ethtool. > > I don't have a good sense of what a good model for cleanup and > permissions is (B). All I know is that if we need to tie things to > processes netlink can do it, and we shouldn't have to create our > own FS and special file descriptors... > > And then there's (C) which is the main part to talk about. > The first step IMHO is to straighten out the configuration process. > Currently we do: > > user -> thin ethtool API --------------------> driver > netdev core <---' > > By "straighten" I mean more of a: > > user -> thin ethtool API ---> netdev core ---> driver > > flow. This means core maintains the full expected configuration, > queue count and their parameters and driver creates those queues > as instructed. > > I'd imagine we'd need 4 basic ops: > - queue_mem_alloc(dev, cfg) -> queue_mem > - queue_mem_free(dev, cfg, queue_mem) > - queue_start(dev, queue info, cfg, queue_mem) -> errno > - queue_stop(dev, queue info, cfg) > > The mem_alloc/mem_free takes care of the commonly missed requirement to > not take the datapath down until resources are allocated for new config. > > Core then sets all the queues up after ndo_open, and tears down before > ndo_stop. In case of an ethtool -L / -G call or enabling / disabling XDP > core can handle the entire reconfiguration dance. > > The cfg object needs to contain all queue configuration, including > the page pool parameters. > > If we have an abstract model of the configuration in the core we can > modify it much more easily, I hope. I mean - the configuration will be > somewhat detached from what's instantiated in the drivers. > > I'd prefer to go as far as we can without introducing a driver callback > to "check if it can support a config change", and try to rely on > (static) capabilities instead. This allows more of the validation to > happen in the core and also lends itself naturally to exporting the > capabilities to the user. > > Checking the use cases: > > #1 - partitioning for containers - storing the cfg in the core gives > us a neat ability to allow users to set the configuration on RSS > context > #2, #3 - page pools - we can make page_pool_create take cfg and read whatever > params we want from there, memory provider, descriptor count, recycling > ring size etc. Also for header-data-split we may want different settings > per queue so again cfg comes in handy > #4 - XDP redirect target - we should spawn XDP TX queues independently from > the XDP configuration > > That's all I have thought up in terms of direction. > Does that make sense? What are the main gaps? Other proposals? More on (A) and (B): I expect most use cases match the containerization that you mention. Where a privileged process handles configuration. For that, the existing interfaces of ethtool -G/-L-/N/-K/-X suffice. A more far-out approach could infer the ntuple 5-tuple connection or 3-tuple listener rule from a socket itself, no ethtool required. But let's ignore that for now. Currently we need to use ethtool -X to restrict the RSS indirection table to a subset of queues. It is not strictly necessary to reconfigure the device on each new container, if pre-allocation a sufficient set of non-RSS queues. Then only ethtool -N is needed to drive data towards one of the non-RSS queues. Or one of the non context 0 RSS contexts if that is used. The main part that is missing is memory allocation. Memory is stranded on unused queues, and there is no explicit support for special allocators. A poor man's solution might be to load a ring with minimal sized buffers (assuming devices accept that, say a zero length buffer), attach a memory provider before inserting an ntuple rule, and refill from the memory provider. This requires accepting that a whole ring of packets is lost before refilled slots get filled.. (I'm messing with that with AF_XDP right now: a process that xsk_binds before filling the FILL queue..) Ideally, we would have a way to reconfigure a single queue, without having to down/up the entire device.. I don't know if the kernel needs an explicit abstract model, or can leave that to the userspace privileged daemon that presses the ethtool buttons.
[ sorry for the delayed response; very busy 2 days ] On 8/16/23 10:12 AM, Willem de Bruijn wrote: > Jakub Kicinski wrote: >> On Sun, 13 Aug 2023 19:10:35 -0600 David Ahern wrote: >>> Also, this suggests that the Rx queue is unique to the flow. I do not >>> recall a netdev API to create H/W queues on the fly (only a passing >>> comment from Kuba), so how is the H/W queue (or queue set since a >>> completion queue is needed as well) created for the flow? >>> And in turn if it is unique to the flow, what deletes the queue if >>> an app does not do a proper cleanup? If the queue sticks around, >>> the dmabuf references stick around. >> >> Let's start sketching out the design for queue config. >> Without sliding into scope creep, hopefully. >> >> Step one - I think we can decompose the problem into: >> A) flow steering >> B) object lifetime and permissions >> C) queue configuration (incl. potentially creating / destroying queues) >> >> These come together into use scenarios like: >> #1 - partitioning for containers - when high perf containers share >> a machine each should get an RSS context on the physical NIC >> to have predictable traffic<>CPU placement, they may also have >> different preferences on how the queues are configured, maybe >> XDP, too? subfunctions are a more effective and simpler solution for containers, no? >> #2 - fancy page pools within the host (e.g. huge pages) >> #3 - very fancy page pools not within the host (Mina's work) >> #4 - XDP redirect target (allowing XDP_REDIRECT without installing XDP >> on the target) >> #5 - busy polling - admittedly a bit theoretical, I don't know of >> anyone busy polling in real life, but one of the problems today >> is that setting it up requires scraping random bits of info from >> sysfs and a lot of hoping. >> >> Flow steering (A) is there today, to a sufficient extent, I think, >> so we can defer on that. Sooner or later we should probably figure >> out if we want to continue down the unruly path of TC offloads or >> just give up and beef up ethtool. Flow steering to TC offloads -- more details on what you were thinking here? >> >> I don't have a good sense of what a good model for cleanup and >> permissions is (B). All I know is that if we need to tie things to >> processes netlink can do it, and we shouldn't have to create our >> own FS and special file descriptors... From my perspective the main sticking point that has not been handled is flushing buffers from the RxQ, but there is 100% tied to queue management and a process' ability to effect a flush or queue tear down - and that is the focus of your list below: >> >> And then there's (C) which is the main part to talk about. >> The first step IMHO is to straighten out the configuration process. >> Currently we do: >> >> user -> thin ethtool API --------------------> driver >> netdev core <---' >> >> By "straighten" I mean more of a: >> >> user -> thin ethtool API ---> netdev core ---> driver >> >> flow. This means core maintains the full expected configuration, >> queue count and their parameters and driver creates those queues >> as instructed. >> >> I'd imagine we'd need 4 basic ops: >> - queue_mem_alloc(dev, cfg) -> queue_mem >> - queue_mem_free(dev, cfg, queue_mem) >> - queue_start(dev, queue info, cfg, queue_mem) -> errno >> - queue_stop(dev, queue info, cfg) >> >> The mem_alloc/mem_free takes care of the commonly missed requirement to >> not take the datapath down until resources are allocated for new config. sounds reasonable. >> >> Core then sets all the queues up after ndo_open, and tears down before >> ndo_stop. In case of an ethtool -L / -G call or enabling / disabling XDP >> core can handle the entire reconfiguration dance. `ethtool -L/-G` and `ip link set {up/down}` pertain to the "general OS" queues managed by a driver for generic workloads and networking management (e.g., neigh discovery, icmp, etc). The discussions here pertains to processes wanting to use their own memory or GPU memory in a queue. Processes will come and go and the queue management needs to align with that need without affecting all of the other queues managed by the driver. >> >> The cfg object needs to contain all queue configuration, including >> the page pool parameters. >> >> If we have an abstract model of the configuration in the core we can >> modify it much more easily, I hope. I mean - the configuration will be >> somewhat detached from what's instantiated in the drivers. >> >> I'd prefer to go as far as we can without introducing a driver callback >> to "check if it can support a config change", and try to rely on >> (static) capabilities instead. This allows more of the validation to >> happen in the core and also lends itself naturally to exporting the >> capabilities to the user. >> >> Checking the use cases: >> >> #1 - partitioning for containers - storing the cfg in the core gives >> us a neat ability to allow users to set the configuration on RSS >> context >> #2, #3 - page pools - we can make page_pool_create take cfg and read whatever >> params we want from there, memory provider, descriptor count, recycling >> ring size etc. Also for header-data-split we may want different settings >> per queue so again cfg comes in handy >> #4 - XDP redirect target - we should spawn XDP TX queues independently from >> the XDP configuration >> >> That's all I have thought up in terms of direction. >> Does that make sense? What are the main gaps? Other proposals? > > More on (A) and (B): > > I expect most use cases match the containerization that you mention. > Where a privileged process handles configuration. > > For that, the existing interfaces of ethtool -G/-L-/N/-K/-X suffice. > > A more far-out approach could infer the ntuple 5-tuple connection or > 3-tuple listener rule from a socket itself, no ethtool required. But > let's ignore that for now. > > Currently we need to use ethtool -X to restrict the RSS indirection > table to a subset of queues. It is not strictly necessary to > reconfigure the device on each new container, if pre-allocation a > sufficient set of non-RSS queues. This is an interesting approach: This scheme here is along the lines of you have N cpus in the server, so N queue sets (or channels). The indirection table means M queue sets are used for RSS leaving N-M queues for flows with "fancy memory providers". Such a model can work but it is quite passive, needs careful orchestration and has a lot of moving, disjointed pieces - with some race conditions around setup vs first data packet arriving. I was thinking about a more generic design where H/W queues are created and destroyed - e.g., queues unique to a process which makes the cleanup so much easier. > > Then only ethtool -N is needed to drive data towards one of the > non-RSS queues. Or one of the non context 0 RSS contexts if that is > used. > > The main part that is missing is memory allocation. Memory is stranded > on unused queues, and there is no explicit support for special > allocators. > > A poor man's solution might be to load a ring with minimal sized > buffers (assuming devices accept that, say a zero length buffer), > attach a memory provider before inserting an ntuple rule, and refill > from the memory provider. This requires accepting that a whole ring of > packets is lost before refilled slots get filled.. > > (I'm messing with that with AF_XDP right now: a process that xsk_binds > before filling the FILL queue..) > > Ideally, we would have a way to reconfigure a single queue, without > having to down/up the entire device.. > > I don't know if the kernel needs an explicit abstract model, or can > leave that to the userspace privileged daemon that presses the ethtool > buttons. The kernel has that in the IB verbs S/W APIs. Yes, I realize that comment amounts to profanity on this mailing list, but it should not be. There are existing APIs for creating, managing and destroying queues - open source, GPL'ed, *software* APIs that are open for all to use. That said, I have no religion here. If the netdev stack wants new APIs to manage queues - including supplying buffers - drivers will have APIs that can be adapted to some new ndo to create, configure, and destroy queues. The ethtool API can be updated to manage that. Ultimately I believe anything short of dynamic queue management will be a band-aid approach that will have a lot of problems.
On Thu, 17 Aug 2023 19:33:47 -0600 David Ahern wrote: > [ sorry for the delayed response; very busy 2 days ] Tell me about it :) > On 8/16/23 10:12 AM, Willem de Bruijn wrote: > > Jakub Kicinski wrote: > >> Let's start sketching out the design for queue config. > >> Without sliding into scope creep, hopefully. > >> > >> Step one - I think we can decompose the problem into: > >> A) flow steering > >> B) object lifetime and permissions > >> C) queue configuration (incl. potentially creating / destroying queues) > >> > >> These come together into use scenarios like: > >> #1 - partitioning for containers - when high perf containers share > >> a machine each should get an RSS context on the physical NIC > >> to have predictable traffic<>CPU placement, they may also have > >> different preferences on how the queues are configured, maybe > >> XDP, too? > > subfunctions are a more effective and simpler solution for containers, no? Maybe, subfunctions offload a lot, let's not go too far into the weeds on production and flexibility considerations but they wouldn't be my first choice. > >> #2 - fancy page pools within the host (e.g. huge pages) > >> #3 - very fancy page pools not within the host (Mina's work) > >> #4 - XDP redirect target (allowing XDP_REDIRECT without installing XDP > >> on the target) > >> #5 - busy polling - admittedly a bit theoretical, I don't know of > >> anyone busy polling in real life, but one of the problems today > >> is that setting it up requires scraping random bits of info from > >> sysfs and a lot of hoping. > >> > >> Flow steering (A) is there today, to a sufficient extent, I think, > >> so we can defer on that. Sooner or later we should probably figure > >> out if we want to continue down the unruly path of TC offloads or > >> just give up and beef up ethtool. > > Flow steering to TC offloads -- more details on what you were thinking here? I think TC flower can do almost everything ethtool -N can. So do we continue to developer for both APIs or pick one? > >> I don't have a good sense of what a good model for cleanup and > >> permissions is (B). All I know is that if we need to tie things to > >> processes netlink can do it, and we shouldn't have to create our > >> own FS and special file descriptors... > > From my perspective the main sticking point that has not been handled is > flushing buffers from the RxQ, but there is 100% tied to queue > management and a process' ability to effect a flush or queue tear down - > and that is the focus of your list below: If you're thinking about it from the perspective of "application died give me back all the buffers" - the RxQ is just one piece, right? As we discovered with page pool - packets may get stuck in stack for ever. > >> And then there's (C) which is the main part to talk about. > >> The first step IMHO is to straighten out the configuration process. > >> Currently we do: > >> > >> user -> thin ethtool API --------------------> driver > >> netdev core <---' > >> > >> By "straighten" I mean more of a: > >> > >> user -> thin ethtool API ---> netdev core ---> driver > >> > >> flow. This means core maintains the full expected configuration, > >> queue count and their parameters and driver creates those queues > >> as instructed. > >> > >> I'd imagine we'd need 4 basic ops: > >> - queue_mem_alloc(dev, cfg) -> queue_mem > >> - queue_mem_free(dev, cfg, queue_mem) > >> - queue_start(dev, queue info, cfg, queue_mem) -> errno > >> - queue_stop(dev, queue info, cfg) > >> > >> The mem_alloc/mem_free takes care of the commonly missed requirement to > >> not take the datapath down until resources are allocated for new config. > > sounds reasonable. > > >> > >> Core then sets all the queues up after ndo_open, and tears down before > >> ndo_stop. In case of an ethtool -L / -G call or enabling / disabling XDP > >> core can handle the entire reconfiguration dance. > > `ethtool -L/-G` and `ip link set {up/down}` pertain to the "general OS" > queues managed by a driver for generic workloads and networking > management (e.g., neigh discovery, icmp, etc). The discussions here > pertains to processes wanting to use their own memory or GPU memory in a > queue. Processes will come and go and the queue management needs to > align with that need without affecting all of the other queues managed > by the driver. For sure, I'm just saying that both the old uAPI can be translated to the new driver API, and so should the new uAPIs. I focused on the driver facing APIs because I think that it's the hard part. We have many drivers, the uAPI is more easily dreamed up, no? > >> The cfg object needs to contain all queue configuration, including > >> the page pool parameters. > >> > >> If we have an abstract model of the configuration in the core we can > >> modify it much more easily, I hope. I mean - the configuration will be > >> somewhat detached from what's instantiated in the drivers. > >> > >> I'd prefer to go as far as we can without introducing a driver callback > >> to "check if it can support a config change", and try to rely on > >> (static) capabilities instead. This allows more of the validation to > >> happen in the core and also lends itself naturally to exporting the > >> capabilities to the user. > >> > >> Checking the use cases: > >> > >> #1 - partitioning for containers - storing the cfg in the core gives > >> us a neat ability to allow users to set the configuration on RSS > >> context > >> #2, #3 - page pools - we can make page_pool_create take cfg and read whatever > >> params we want from there, memory provider, descriptor count, recycling > >> ring size etc. Also for header-data-split we may want different settings > >> per queue so again cfg comes in handy > >> #4 - XDP redirect target - we should spawn XDP TX queues independently from > >> the XDP configuration > >> > >> That's all I have thought up in terms of direction. > >> Does that make sense? What are the main gaps? Other proposals? > > > > More on (A) and (B): > > > > I expect most use cases match the containerization that you mention. > > Where a privileged process handles configuration. > > > > For that, the existing interfaces of ethtool -G/-L-/N/-K/-X suffice. > > > > A more far-out approach could infer the ntuple 5-tuple connection or > > 3-tuple listener rule from a socket itself, no ethtool required. But > > let's ignore that for now. > > > > Currently we need to use ethtool -X to restrict the RSS indirection > > table to a subset of queues. It is not strictly necessary to > > reconfigure the device on each new container, if pre-allocation a > > sufficient set of non-RSS queues. > > This is an interesting approach: This scheme here is along the lines of > you have N cpus in the server, so N queue sets (or channels). The > indirection table means M queue sets are used for RSS leaving N-M queues > for flows with "fancy memory providers". Such a model can work but it is > quite passive, needs careful orchestration and has a lot of moving, > disjointed pieces - with some race conditions around setup vs first data > packet arriving. > > I was thinking about a more generic design where H/W queues are created > and destroyed - e.g., queues unique to a process which makes the cleanup > so much easier. FWIW what Willem describes is what we were telling people to do for AF_XDP for however many years it existed. > > Then only ethtool -N is needed to drive data towards one of the > > non-RSS queues. Or one of the non context 0 RSS contexts if that is > > used. > > > > The main part that is missing is memory allocation. Memory is stranded > > on unused queues, and there is no explicit support for special > > allocators. > > > > A poor man's solution might be to load a ring with minimal sized > > buffers (assuming devices accept that, say a zero length buffer), > > attach a memory provider before inserting an ntuple rule, and refill > > from the memory provider. This requires accepting that a whole ring of > > packets is lost before refilled slots get filled.. > > > > (I'm messing with that with AF_XDP right now: a process that xsk_binds > > before filling the FILL queue..) > > > > Ideally, we would have a way to reconfigure a single queue, without > > having to down/up the entire device.. > > > > I don't know if the kernel needs an explicit abstract model, or can > > leave that to the userspace privileged daemon that presses the ethtool > > buttons. > > The kernel has that in the IB verbs S/W APIs. Yes, I realize that > comment amounts to profanity on this mailing list, but it should not be. > There are existing APIs for creating, managing and destroying queues - > open source, GPL'ed, *software* APIs that are open for all to use. > > That said, I have no religion here. If the netdev stack wants new APIs > to manage queues - including supplying buffers - drivers will have APIs > that can be adapted to some new ndo to create, configure, and destroy > queues. The ethtool API can be updated to manage that. Ultimately I > believe anything short of dynamic queue management will be a band-aid > approach that will have a lot of problems. No religion here either, but the APIs we talk about are not particularly complex. Having started hacking things together with page pools, huge pages, RSS etc - IMHO the reuse and convergence would be very superficial.
On 8/17/23 8:09 PM, Jakub Kicinski wrote: >> >> Flow steering to TC offloads -- more details on what you were thinking here? > > I think TC flower can do almost everything ethtool -N can. > So do we continue to developer for both APIs or pick one? ok, tc flower; that did not come to mind. Don't use it often. > >>>> I don't have a good sense of what a good model for cleanup and >>>> permissions is (B). All I know is that if we need to tie things to >>>> processes netlink can do it, and we shouldn't have to create our >>>> own FS and special file descriptors... >> >> From my perspective the main sticking point that has not been handled is >> flushing buffers from the RxQ, but there is 100% tied to queue >> management and a process' ability to effect a flush or queue tear down - >> and that is the focus of your list below: > > If you're thinking about it from the perspective of "application died > give me back all the buffers" - the RxQ is just one piece, right? > As we discovered with page pool - packets may get stuck in stack for > ever. Yes, flushing the retransmit queue for TCP is one of those places where buffer references can get stuck for some amount of time. >> >> `ethtool -L/-G` and `ip link set {up/down}` pertain to the "general OS" >> queues managed by a driver for generic workloads and networking >> management (e.g., neigh discovery, icmp, etc). The discussions here >> pertains to processes wanting to use their own memory or GPU memory in a >> queue. Processes will come and go and the queue management needs to >> align with that need without affecting all of the other queues managed >> by the driver. > > For sure, I'm just saying that both the old uAPI can be translated to > the new driver API, and so should the new uAPIs. I focused on the > driver facing APIs because I think that it's the hard part. We have > many drivers, the uAPI is more easily dreamed up, no? sure.
On Thu, Aug 17, 2023 at 7:10 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 17 Aug 2023 19:33:47 -0600 David Ahern wrote: > > [ sorry for the delayed response; very busy 2 days ] > > Tell me about it :) > > > On 8/16/23 10:12 AM, Willem de Bruijn wrote: > > > Jakub Kicinski wrote: > > >> Let's start sketching out the design for queue config. > > >> Without sliding into scope creep, hopefully. > > >> > > >> Step one - I think we can decompose the problem into: > > >> A) flow steering > > >> B) object lifetime and permissions > > >> C) queue configuration (incl. potentially creating / destroying queues) > > >> > > >> These come together into use scenarios like: > > >> #1 - partitioning for containers - when high perf containers share > > >> a machine each should get an RSS context on the physical NIC > > >> to have predictable traffic<>CPU placement, they may also have > > >> different preferences on how the queues are configured, maybe > > >> XDP, too? > > > > subfunctions are a more effective and simpler solution for containers, no? > > Maybe, subfunctions offload a lot, let's not go too far into the weeds > on production and flexibility considerations but they wouldn't be my > first choice. > > > >> #2 - fancy page pools within the host (e.g. huge pages) > > >> #3 - very fancy page pools not within the host (Mina's work) > > >> #4 - XDP redirect target (allowing XDP_REDIRECT without installing XDP > > >> on the target) > > >> #5 - busy polling - admittedly a bit theoretical, I don't know of > > >> anyone busy polling in real life, but one of the problems today > > >> is that setting it up requires scraping random bits of info from > > >> sysfs and a lot of hoping. > > >> > > >> Flow steering (A) is there today, to a sufficient extent, I think, > > >> so we can defer on that. Sooner or later we should probably figure > > >> out if we want to continue down the unruly path of TC offloads or > > >> just give up and beef up ethtool. > > > > Flow steering to TC offloads -- more details on what you were thinking here? > > I think TC flower can do almost everything ethtool -N can. > So do we continue to developer for both APIs or pick one? > > > >> I don't have a good sense of what a good model for cleanup and > > >> permissions is (B). All I know is that if we need to tie things to > > >> processes netlink can do it, and we shouldn't have to create our > > >> own FS and special file descriptors... > > > > From my perspective the main sticking point that has not been handled is > > flushing buffers from the RxQ, but there is 100% tied to queue > > management and a process' ability to effect a flush or queue tear down - > > and that is the focus of your list below: > > If you're thinking about it from the perspective of "application died > give me back all the buffers" - the RxQ is just one piece, right? > As we discovered with page pool - packets may get stuck in stack for > ever. > > > >> And then there's (C) which is the main part to talk about. > > >> The first step IMHO is to straighten out the configuration process. > > >> Currently we do: > > >> > > >> user -> thin ethtool API --------------------> driver > > >> netdev core <---' > > >> > > >> By "straighten" I mean more of a: > > >> > > >> user -> thin ethtool API ---> netdev core ---> driver > > >> > > >> flow. This means core maintains the full expected configuration, > > >> queue count and their parameters and driver creates those queues > > >> as instructed. > > >> > > >> I'd imagine we'd need 4 basic ops: > > >> - queue_mem_alloc(dev, cfg) -> queue_mem > > >> - queue_mem_free(dev, cfg, queue_mem) > > >> - queue_start(dev, queue info, cfg, queue_mem) -> errno > > >> - queue_stop(dev, queue info, cfg) > > >> > > >> The mem_alloc/mem_free takes care of the commonly missed requirement to > > >> not take the datapath down until resources are allocated for new config. > > > > sounds reasonable. > > Thanks for taking the time to review & provide suggestions. I do need to understand concrete changes to apply to the next revision. Here is my understanding so far, please correct if wrong, and sorry if I didn't capture everything you want: The sticking points are: 1. From David: this proposal doesn't give an application the ability to flush an rx queue, which means that we have to rely on a driver reset that affects all queues to refill the rx queue buffers. 2. From Jakub: the uAPI and implementation here needs to be in line with his general direction & extensible to apply to existing use cases `ethtool -L/-G`, etc. AFAIU this is what I need to do in the next version: 1. The uAPI will be changed such that it will either re-configure an existing queue to bind it to the dma-buf, or allocate a new queue bound to the dma-buf (not sure which is better at the moment). Either way, the configuration will take place immediately, and not rely on an entire driver reset to actuate the change. 2. The uAPI will be changed such that if the netlink socket is closed, or the process dies, the rx queue will be unbound from the dma-buf or the rx queue will be freed entirely (again, not sure which is better at the moment). The configuration will take place immediately without relying on a driver reset. 3. I will add 4 new net_device_ops that Jakub specified: queue_mem_alloc/free(), and queue_start/stop(). 4. The uAPI mentioned in #1 will use the new net_device_ops to allocate or reconfigure a queue attached to the provided dma-buf. Does this sound roughly reasonable here? AFAICT the only technical difficulty is that I'm not sure it's feasible for a driver to start or stop 1 rx-queue without triggering a full driver reset. The (2) drivers I looked at both do a full reset to change any queue configuration. I'll investigate. > > >> > > >> Core then sets all the queues up after ndo_open, and tears down before > > >> ndo_stop. In case of an ethtool -L / -G call or enabling / disabling XDP > > >> core can handle the entire reconfiguration dance. > > > > `ethtool -L/-G` and `ip link set {up/down}` pertain to the "general OS" > > queues managed by a driver for generic workloads and networking > > management (e.g., neigh discovery, icmp, etc). The discussions here > > pertains to processes wanting to use their own memory or GPU memory in a > > queue. Processes will come and go and the queue management needs to > > align with that need without affecting all of the other queues managed > > by the driver. > > For sure, I'm just saying that both the old uAPI can be translated to > the new driver API, and so should the new uAPIs. I focused on the > driver facing APIs because I think that it's the hard part. We have > many drivers, the uAPI is more easily dreamed up, no? > > > >> The cfg object needs to contain all queue configuration, including > > >> the page pool parameters. > > >> > > >> If we have an abstract model of the configuration in the core we can > > >> modify it much more easily, I hope. I mean - the configuration will be > > >> somewhat detached from what's instantiated in the drivers. > > >> > > >> I'd prefer to go as far as we can without introducing a driver callback > > >> to "check if it can support a config change", and try to rely on > > >> (static) capabilities instead. This allows more of the validation to > > >> happen in the core and also lends itself naturally to exporting the > > >> capabilities to the user. > > >> > > >> Checking the use cases: > > >> > > >> #1 - partitioning for containers - storing the cfg in the core gives > > >> us a neat ability to allow users to set the configuration on RSS > > >> context > > >> #2, #3 - page pools - we can make page_pool_create take cfg and read whatever > > >> params we want from there, memory provider, descriptor count, recycling > > >> ring size etc. Also for header-data-split we may want different settings > > >> per queue so again cfg comes in handy > > >> #4 - XDP redirect target - we should spawn XDP TX queues independently from > > >> the XDP configuration > > >> > > >> That's all I have thought up in terms of direction. > > >> Does that make sense? What are the main gaps? Other proposals? > > > > > > More on (A) and (B): > > > > > > I expect most use cases match the containerization that you mention. > > > Where a privileged process handles configuration. > > > > > > For that, the existing interfaces of ethtool -G/-L-/N/-K/-X suffice. > > > > > > A more far-out approach could infer the ntuple 5-tuple connection or > > > 3-tuple listener rule from a socket itself, no ethtool required. But > > > let's ignore that for now. > > > > > > Currently we need to use ethtool -X to restrict the RSS indirection > > > table to a subset of queues. It is not strictly necessary to > > > reconfigure the device on each new container, if pre-allocation a > > > sufficient set of non-RSS queues. > > > > This is an interesting approach: This scheme here is along the lines of > > you have N cpus in the server, so N queue sets (or channels). The > > indirection table means M queue sets are used for RSS leaving N-M queues > > for flows with "fancy memory providers". Such a model can work but it is > > quite passive, needs careful orchestration and has a lot of moving, > > disjointed pieces - with some race conditions around setup vs first data > > packet arriving. > > > > I was thinking about a more generic design where H/W queues are created > > and destroyed - e.g., queues unique to a process which makes the cleanup > > so much easier. > > FWIW what Willem describes is what we were telling people to do for > AF_XDP for however many years it existed. > > > > Then only ethtool -N is needed to drive data towards one of the > > > non-RSS queues. Or one of the non context 0 RSS contexts if that is > > > used. > > > > > > The main part that is missing is memory allocation. Memory is stranded > > > on unused queues, and there is no explicit support for special > > > allocators. > > > > > > A poor man's solution might be to load a ring with minimal sized > > > buffers (assuming devices accept that, say a zero length buffer), > > > attach a memory provider before inserting an ntuple rule, and refill > > > from the memory provider. This requires accepting that a whole ring of > > > packets is lost before refilled slots get filled.. > > > > > > (I'm messing with that with AF_XDP right now: a process that xsk_binds > > > before filling the FILL queue..) > > > > > > Ideally, we would have a way to reconfigure a single queue, without > > > having to down/up the entire device.. > > > > > > I don't know if the kernel needs an explicit abstract model, or can > > > leave that to the userspace privileged daemon that presses the ethtool > > > buttons. > > > > The kernel has that in the IB verbs S/W APIs. Yes, I realize that > > comment amounts to profanity on this mailing list, but it should not be. > > There are existing APIs for creating, managing and destroying queues - > > open source, GPL'ed, *software* APIs that are open for all to use. > > > > That said, I have no religion here. If the netdev stack wants new APIs > > to manage queues - including supplying buffers - drivers will have APIs > > that can be adapted to some new ndo to create, configure, and destroy > > queues. The ethtool API can be updated to manage that. Ultimately I > > believe anything short of dynamic queue management will be a band-aid > > approach that will have a lot of problems. > > No religion here either, but the APIs we talk about are not > particularly complex. Having started hacking things together > with page pools, huge pages, RSS etc - IMHO the reuse and convergence > would be very superficial.
On 8/18/23 3:52 PM, Mina Almasry wrote: > The sticking points are: > 1. From David: this proposal doesn't give an application the ability > to flush an rx queue, which means that we have to rely on a driver > reset that affects all queues to refill the rx queue buffers. Generically, the design needs to be able to flush (or invalidate) all references to the dma-buf once the process no longer "owns" it. > 2. From Jakub: the uAPI and implementation here needs to be in line > with his general direction & extensible to apply to existing use cases > `ethtool -L/-G`, etc. I think this is a bit more open ended given the openness of the netdev netlink API. i.e., managing a H/W queue (create, delete, stop / flush, associate a page_pool) could be done through this API. > > AFAIU this is what I need to do in the next version: > > 1. The uAPI will be changed such that it will either re-configure an > existing queue to bind it to the dma-buf, or allocate a new queue > bound to the dma-buf (not sure which is better at the moment). Either 1. API to manage a page-pool (create, delete, update). 2. API to add and remove a dma-buf (or host memory buffer) with a page-pool. Remove may take time to flush references pushed to hardware so this would be asynchronous. 3. Create a queue or use an existing queue id and associate a page-pool with it. > way, the configuration will take place immediately, and not rely on an > entire driver reset to actuate the change. yes > > 2. The uAPI will be changed such that if the netlink socket is closed, > or the process dies, the rx queue will be unbound from the dma-buf or > the rx queue will be freed entirely (again, not sure which is better I think those are separate actions. But, if the queue was created by and referenced by a process, then closing an fd means it should be freed. > at the moment). The configuration will take place immediately without > relying on a driver reset. yes on the reset. > > 3. I will add 4 new net_device_ops that Jakub specified: > queue_mem_alloc/free(), and queue_start/stop(). > > 4. The uAPI mentioned in #1 will use the new net_device_ops to > allocate or reconfigure a queue attached to the provided dma-buf. > > Does this sound roughly reasonable here? > > AFAICT the only technical difficulty is that I'm not sure it's > feasible for a driver to start or stop 1 rx-queue without triggering a > full driver reset. The (2) drivers I looked at both do a full reset to > change any queue configuration. I'll investigate.
On Fri, 18 Aug 2023 19:34:32 -0600 David Ahern wrote: > On 8/18/23 3:52 PM, Mina Almasry wrote: > > The sticking points are: > > 1. From David: this proposal doesn't give an application the ability > > to flush an rx queue, which means that we have to rely on a driver > > reset that affects all queues to refill the rx queue buffers. > > Generically, the design needs to be able to flush (or invalidate) all > references to the dma-buf once the process no longer "owns" it. Are we talking about the ability for the app to flush the queue when it wants to (do no idea what)? Or auto-flush when app crashes? > > 2. From Jakub: the uAPI and implementation here needs to be in line > > with his general direction & extensible to apply to existing use cases > > `ethtool -L/-G`, etc. > > I think this is a bit more open ended given the openness of the netdev > netlink API. i.e., managing a H/W queue (create, delete, stop / flush, > associate a page_pool) could be done through this API. > > > > > AFAIU this is what I need to do in the next version: > > > > 1. The uAPI will be changed such that it will either re-configure an > > existing queue to bind it to the dma-buf, or allocate a new queue > > bound to the dma-buf (not sure which is better at the moment). Either > > 1. API to manage a page-pool (create, delete, update). I wasn't anticipating a "create page pool" API. I was thinking of a scheme where user space sets page pool parameters, but the driver still creates the pool. But I guess it is doable. More work, tho. Are there ibverbs which can do it? lol. > 2. API to add and remove a dma-buf (or host memory buffer) with a > page-pool. Remove may take time to flush references pushed to hardware > so this would be asynchronous. > > 3. Create a queue or use an existing queue id and associate a page-pool > with it. > > > way, the configuration will take place immediately, and not rely on an > > entire driver reset to actuate the change. > > yes > > > > > 2. The uAPI will be changed such that if the netlink socket is closed, > > or the process dies, the rx queue will be unbound from the dma-buf or > > the rx queue will be freed entirely (again, not sure which is better > > I think those are separate actions. But, if the queue was created by and > referenced by a process, then closing an fd means it should be freed. > > > at the moment). The configuration will take place immediately without > > relying on a driver reset. > > yes on the reset. > > > > > 3. I will add 4 new net_device_ops that Jakub specified: > > queue_mem_alloc/free(), and queue_start/stop(). > > > > 4. The uAPI mentioned in #1 will use the new net_device_ops to > > allocate or reconfigure a queue attached to the provided dma-buf. I'd leave 2, 3, 4 alone for now. Focus on binding a page pool to an existing queue. > > Does this sound roughly reasonable here? > > > > AFAICT the only technical difficulty is that I'm not sure it's > > feasible for a driver to start or stop 1 rx-queue without triggering a > > full driver reset. The (2) drivers I looked at both do a full reset to > > change any queue configuration. I'll investigate. >
On 8/18/23 8:06 PM, Jakub Kicinski wrote: > On Fri, 18 Aug 2023 19:34:32 -0600 David Ahern wrote: >> On 8/18/23 3:52 PM, Mina Almasry wrote: >>> The sticking points are: >>> 1. From David: this proposal doesn't give an application the ability >>> to flush an rx queue, which means that we have to rely on a driver >>> reset that affects all queues to refill the rx queue buffers. >> >> Generically, the design needs to be able to flush (or invalidate) all >> references to the dma-buf once the process no longer "owns" it. > > Are we talking about the ability for the app to flush the queue > when it wants to (do no idea what)? Or auto-flush when app crashes? If a buffer reference can be invalidated such that a posted buffer is ignored by H/W, then no flush is needed per se. Either way the key point is that posted buffers can no longer be filled by H/W once a process no longer owns the dma-buf reference. I believe the actual mechanism here will vary by H/W. > >>> 2. From Jakub: the uAPI and implementation here needs to be in line >>> with his general direction & extensible to apply to existing use cases >>> `ethtool -L/-G`, etc. >> >> I think this is a bit more open ended given the openness of the netdev >> netlink API. i.e., managing a H/W queue (create, delete, stop / flush, >> associate a page_pool) could be done through this API. >> >>> >>> AFAIU this is what I need to do in the next version: >>> >>> 1. The uAPI will be changed such that it will either re-configure an >>> existing queue to bind it to the dma-buf, or allocate a new queue >>> bound to the dma-buf (not sure which is better at the moment). Either >> >> 1. API to manage a page-pool (create, delete, update). > > I wasn't anticipating a "create page pool" API. > > I was thinking of a scheme where user space sets page pool parameters, > but the driver still creates the pool. There needs to be a process (or process group depending on design) unique page pool due to the lifetime of what is backing it. The driver or core netdev code can create it; if it is tied to an rx queue and created by the driver there are design implications. As separate objects page pools and rx queues can have their own lifetimes (e.g., multiple rx queues for a single pp); generically a H/W queue and the basis of buffers supplied to land packets are independent. > > But I guess it is doable. More work, tho. Are there ibverbs which > can do it? lol. Well, generically yes - think intent not necessarily a 1:1 mapping. > >> 2. API to add and remove a dma-buf (or host memory buffer) with a >> page-pool. Remove may take time to flush references pushed to hardware >> so this would be asynchronous. >> >> 3. Create a queue or use an existing queue id and associate a page-pool >> with it. >> >>> way, the configuration will take place immediately, and not rely on an >>> entire driver reset to actuate the change. >> >> yes >> >>> >>> 2. The uAPI will be changed such that if the netlink socket is closed, >>> or the process dies, the rx queue will be unbound from the dma-buf or >>> the rx queue will be freed entirely (again, not sure which is better >> >> I think those are separate actions. But, if the queue was created by and >> referenced by a process, then closing an fd means it should be freed. >> >>> at the moment). The configuration will take place immediately without >>> relying on a driver reset. >> >> yes on the reset. >> >>> >>> 3. I will add 4 new net_device_ops that Jakub specified: >>> queue_mem_alloc/free(), and queue_start/stop(). >>> >>> 4. The uAPI mentioned in #1 will use the new net_device_ops to >>> allocate or reconfigure a queue attached to the provided dma-buf. > > I'd leave 2, 3, 4 alone for now. Focus on binding a page pool to > an existing queue. > >>> Does this sound roughly reasonable here? >>> >>> AFAICT the only technical difficulty is that I'm not sure it's >>> feasible for a driver to start or stop 1 rx-queue without triggering a >>> full driver reset. The (2) drivers I looked at both do a full reset to >>> change any queue configuration. I'll investigate. >> >
On Fri, Aug 18, 2023 at 11:30 PM David Ahern <dsahern@kernel.org> wrote: > > On 8/18/23 8:06 PM, Jakub Kicinski wrote: > > On Fri, 18 Aug 2023 19:34:32 -0600 David Ahern wrote: > >> On 8/18/23 3:52 PM, Mina Almasry wrote: > >>> The sticking points are: > >>> 1. From David: this proposal doesn't give an application the ability > >>> to flush an rx queue, which means that we have to rely on a driver > >>> reset that affects all queues to refill the rx queue buffers. > >> > >> Generically, the design needs to be able to flush (or invalidate) all > >> references to the dma-buf once the process no longer "owns" it. > > > > Are we talking about the ability for the app to flush the queue > > when it wants to (do no idea what)? Or auto-flush when app crashes? > > If a buffer reference can be invalidated such that a posted buffer is > ignored by H/W, then no flush is needed per se. Either way the key point > is that posted buffers can no longer be filled by H/W once a process no > longer owns the dma-buf reference. I believe the actual mechanism here > will vary by H/W. Right. Many devices only allow bringing all queues down at the same time. Once a descriptor is posted and the ring head is written, there is no way to retract that. Since waiting for the device to catch up is not acceptable, the only option is to bring down the queue, right? Which will imply bringing down the entire device on many devices. Not ideal, but acceptable short term, imho. That may be an incentive for vendors to support per-queue start/stop/alloc/free. Maybe the ones that support RDMA already do?
On Sat, Aug 19, 2023 at 7:19 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Fri, Aug 18, 2023 at 11:30 PM David Ahern <dsahern@kernel.org> wrote: > > > > On 8/18/23 8:06 PM, Jakub Kicinski wrote: > > > On Fri, 18 Aug 2023 19:34:32 -0600 David Ahern wrote: > > >> On 8/18/23 3:52 PM, Mina Almasry wrote: > > >>> The sticking points are: > > >>> 1. From David: this proposal doesn't give an application the ability > > >>> to flush an rx queue, which means that we have to rely on a driver > > >>> reset that affects all queues to refill the rx queue buffers. > > >> > > >> Generically, the design needs to be able to flush (or invalidate) all > > >> references to the dma-buf once the process no longer "owns" it. > > > > > > Are we talking about the ability for the app to flush the queue > > > when it wants to (do no idea what)? Or auto-flush when app crashes? > > > > If a buffer reference can be invalidated such that a posted buffer is > > ignored by H/W, then no flush is needed per se. Either way the key point > > is that posted buffers can no longer be filled by H/W once a process no > > longer owns the dma-buf reference. I believe the actual mechanism here > > will vary by H/W. > > Right. Many devices only allow bringing all queues down at the same time. > FWIW, I spoke with our Praveen (GVE maintainer) about this. Suspicion is that bringing up/down individual queues _should_ work with GVE for the most part, but it's pending me trying it and confirming. I think if a driver can't support bringing up/down individual queues, then Jakub's direction for per queue configs all cannot be done on that driver (queue_mem_alloc, queue_mem_free, queue_start, queue_stop), and addressing David's concerns vis-a-vis dma-buf being auto-detached if the application crashes/exists also cannot be done. The driver will not be able to support device memory TCP unless there is an option to make it work with a full driver reset. > Once a descriptor is posted and the ring head is written, there is no > way to retract that. Since waiting for the device to catch up is not > acceptable, the only option is to bring down the queue, right? Which > will imply bringing down the entire device on many devices. Not ideal, > but acceptable short term, imho. > I also wonder if it may be acceptable to have both modes supported. I.e. (roughly): 1. Add APIs that create an rx-queue bound to a dma-buf. 2. Add APIs that bind an rx-queue to a dma-buf. Drivers that support per-queue allocation/freeing can support and use #1 and can work as David likes. Drivers that cannot allocate or bring up individual queues can only support #2, and trigger a driver-reset to refill or release the dma-buf references. This patch series already implements APIs #2. > That may be an incentive for vendors to support per-queue > start/stop/alloc/free. Maybe the ones that support RDMA already do?
On Sat, 19 Aug 2023 10:18:57 -0400 Willem de Bruijn wrote: > Right. Many devices only allow bringing all queues down at the same time. > > Once a descriptor is posted and the ring head is written, there is no > way to retract that. Since waiting for the device to catch up is not > acceptable, the only option is to bring down the queue, right? Which > will imply bringing down the entire device on many devices. Not ideal, > but acceptable short term, imho. > > That may be an incentive for vendors to support per-queue > start/stop/alloc/free. Maybe the ones that support RDMA already do? Are you talking about HW devices, or virt? I thought most HW made in the last 10 years should be able to take down individual queues :o
On Mon, Aug 21, 2023 at 5:17 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sat, 19 Aug 2023 10:18:57 -0400 Willem de Bruijn wrote: > > Right. Many devices only allow bringing all queues down at the same time. > > > > Once a descriptor is posted and the ring head is written, there is no > > way to retract that. Since waiting for the device to catch up is not > > acceptable, the only option is to bring down the queue, right? Which > > will imply bringing down the entire device on many devices. Not ideal, > > but acceptable short term, imho. > > > > That may be an incentive for vendors to support per-queue > > start/stop/alloc/free. Maybe the ones that support RDMA already do? > > Are you talking about HW devices, or virt? I thought most HW made > in the last 10 years should be able to take down individual queues :o That's great. This is currently mostly encapsulated device-wide behind ndo_close, with code looping over all rx rings, say. Taking a look at one driver, bnxt, it indeed has a per-ring communication exchange with the device, in hwrm_ring_free_send_msg ("/* Flush rings and disable interrupts */"), which is called before the other normal steps: napi disable, dma unmap, posted mem free, irq_release, napi delete and ring mem free. This is what you meant? The issue I was unsure of was quiescing the device immediately, i.e., that hwrm_ring_free_send_msg. I guess this means that this could all be structured on a per-queue basis rather than from ndo_close. Would be a significant change to many drivers, I'd imagine.
On Mon, 21 Aug 2023 20:38:09 -0400 Willem de Bruijn wrote: > > Are you talking about HW devices, or virt? I thought most HW made > > in the last 10 years should be able to take down individual queues :o > > That's great. This is currently mostly encapsulated device-wide behind > ndo_close, with code looping over all rx rings, say. > > Taking a look at one driver, bnxt, it indeed has a per-ring > communication exchange with the device, in hwrm_ring_free_send_msg > ("/* Flush rings and disable interrupts */"), which is called before > the other normal steps: napi disable, dma unmap, posted mem free, > irq_release, napi delete and ring mem free. > > This is what you meant? The issue I was unsure of was quiescing the > device immediately, i.e., that hwrm_ring_free_send_msg. Yes, and I recall we had similar APIs at Netronome for the NFP. I haven't see it in MS specs myself but I wouldn't be surprised if they required it.. There's a bit of an unknown in how well all of this actually works, as the FW/HW paths were not exercised outside of RDMA and potentially other proprietary stuff. > I guess this means that this could all be structured on a per-queue > basis rather than from ndo_close. Would be a significant change to > many drivers, I'd imagine. Yes, it definitely is. The question is how much of it do we require from Mina before merging the mem provider work. I'd really like to avoid the "delegate all the work to the driver" approach that AF_XDP has taken, which is what I'm afraid we'll end up with if we push too hard for a full set of APIs from the start.
On 8/19/23 8:18 AM, Willem de Bruijn wrote: > That may be an incentive for vendors to support per-queue > start/stop/alloc/free. Maybe the ones that support RDMA already do? I looked at most of the H/W RDMA in-tree kernel drivers today, and all of them hand-off create_qp to firmware. I am really surprised by that given that many of those drivers work with netdev. I am fairly certain mlx5 and ConnectX 5-6, for example, can be used to achieve the architecture we proposed at netdev [1], so I was expecting a general queue management interface. Between that and a skim of the providers (userspace side of it), the IB interface may be a dead-end from the perspective of the goals here. [1] https://netdevconf.info/0x16/slides/34/netdev-0x16-Merging-the-Networking-Worlds-slides.pdf, slide 14.
On 2023/8/10 9:57, Mina Almasry wrote: ... > + > +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int dmabuf_fd, > + u32 rxq_idx, struct netdev_dmabuf_binding **out) > +{ > + struct netdev_dmabuf_binding *binding; > + struct netdev_rx_queue *rxq; > + struct scatterlist *sg; > + struct dma_buf *dmabuf; > + unsigned int sg_idx, i; > + unsigned long virtual; > + u32 xa_idx; > + int err; > + > + rxq = __netif_get_rx_queue(dev, rxq_idx); > + > + if (rxq->binding) > + return -EEXIST; > + > + dmabuf = dma_buf_get(dmabuf_fd); > + if (IS_ERR_OR_NULL(dmabuf)) > + return -EBADFD; > + > + binding = kzalloc_node(sizeof(*binding), GFP_KERNEL, > + dev_to_node(&dev->dev)); > + if (!binding) { > + err = -ENOMEM; > + goto err_put_dmabuf; > + } > + > + xa_init_flags(&binding->bound_rxq_list, XA_FLAGS_ALLOC); > + > + refcount_set(&binding->ref, 1); > + > + binding->dmabuf = dmabuf; > + > + binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent); > + if (IS_ERR(binding->attachment)) { > + err = PTR_ERR(binding->attachment); > + goto err_free_binding; > + } > + > + binding->sgt = dma_buf_map_attachment(binding->attachment, > + DMA_BIDIRECTIONAL); > + if (IS_ERR(binding->sgt)) { > + err = PTR_ERR(binding->sgt); > + goto err_detach; > + } > + > + /* 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)); > + if (!binding->chunk_pool) { > + err = -ENOMEM; > + goto err_unmap; > + } > + > + virtual = 0; > + for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) { > + dma_addr_t dma_addr = sg_dma_address(sg); > + struct dmabuf_genpool_chunk_owner *owner; > + size_t len = sg_dma_len(sg); > + struct page_pool_iov *ppiov; > + > + owner = kzalloc_node(sizeof(*owner), GFP_KERNEL, > + dev_to_node(&dev->dev)); > + owner->base_virtual = virtual; > + owner->base_dma_addr = dma_addr; > + owner->num_ppiovs = len / PAGE_SIZE; > + owner->binding = binding; > + > + err = gen_pool_add_owner(binding->chunk_pool, dma_addr, > + dma_addr, len, dev_to_node(&dev->dev), > + owner); > + if (err) { > + err = -EINVAL; > + goto err_free_chunks; > + } > + > + owner->ppiovs = kvmalloc_array(owner->num_ppiovs, > + sizeof(*owner->ppiovs), > + GFP_KERNEL); I guess the 'struct page_pool_iov' is the metadat for each chunk, just like the 'struct page' for each page? Do we want to make it cache line aligned as 'struct page' does, so that there is no cache bouncing between different ppiov when freeing and allocating? And we may be able to get rid of the gen_pool if we add more field to store the dma addree, the binding, etc in 'struct page_pool_iov' as devmem is not usable by other subsystem other than net stack and we can use a big page_pool->ring as the backing for all the devmem chunks to replace the gen_pool, at least for current implemention, dmabuf:page_pool/queue is 1:1. If we want to have the same dmabuf shared by different page_pool, it seems better to implement it in page_pool core instead of specific provider, so that other provider or native page pool can make use of that too. As far as we go here, I am not sure if it is possible and reasonable to reuse 'struct page' used by normal memory as much as possible, and add some specific union fields for devmem like page pool does, so that we can have more common handling between devmem and normal memory? I think we may need to split out metadata used by page pool currently from 'struct page', something like the netmem patch set does, as willy was trying to avoid more users using the 'struct page' directly instead of adding more direct users to it: https://lore.kernel.org/linux-mm/20230111042214.907030-1-willy@infradead.org/ > + if (!owner->ppiovs) { > + err = -ENOMEM; > + goto err_free_chunks; > + } > + > + for (i = 0; i < owner->num_ppiovs; i++) { > + ppiov = &owner->ppiovs[i]; > + ppiov->owner = owner; > + refcount_set(&ppiov->refcount, 1); > + } > + > + dma_addr += len; > + virtual += len; > + } > + > + /* TODO: need to be able to bind to multiple rx queues on the same > + * netdevice. The code should already be able to handle that with > + * minimal changes, but the netlink API currently allows for 1 rx > + * queue. > + */ > + err = xa_alloc(&binding->bound_rxq_list, &xa_idx, rxq, xa_limit_32b, > + GFP_KERNEL); > + if (err) > + goto err_free_chunks; > + > + rxq->binding = binding; > + *out = binding; > + > + return 0; > + > +err_free_chunks: > + gen_pool_for_each_chunk(binding->chunk_pool, > + netdev_devmem_free_chunk_owner, NULL); > + gen_pool_destroy(binding->chunk_pool); > +err_unmap: > + dma_buf_unmap_attachment(binding->attachment, binding->sgt, > + DMA_BIDIRECTIONAL); > +err_detach: > + dma_buf_detach(dmabuf, binding->attachment); > +err_free_binding: > + kfree(binding); > +err_put_dmabuf: > + dma_buf_put(dmabuf); > + return err; > +} > +
On 09/08/2023 18:57, Mina Almasry wrote: > Add a netdev_dmabuf_binding struct which represents the > dma-buf-to-netdevice binding. The netlink API will bind the dma-buf to > an rx queue on the netdevice. On the binding, the dma_buf_attach > & dma_buf_map_attachment will occur. The entries in the sg_table from > mapping will be inserted into a genpool to make it ready > for allocation. > > The chunks in the genpool are owned by a dmabuf_chunk_owner struct which > holds the dma-buf offset of the base of the chunk and the dma_addr of > the chunk. Both are needed to use allocations that come from this chunk. > > We create a new type that represents an allocation from the genpool: > page_pool_iov. We setup the page_pool_iov allocation size in the > genpool to PAGE_SIZE for simplicity: to match the PAGE_SIZE normally > allocated by the page pool and given to the drivers. > > The user can unbind the dmabuf from the netdevice by closing the netlink > socket that established the binding. We do this so that the binding is > automatically unbound even if the userspace process crashes. > > The binding and unbinding leaves an indicator in struct netdev_rx_queue > that the given queue is bound, but the binding doesn't take effect until > the driver actually reconfigures its queues, and re-initializes its page > pool. This issue/weirdness is highlighted in the memory provider > proposal[1], and I'm hoping that some generic solution for all > memory providers will be discussed; this patch doesn't address that > weirdness again. > > The netdev_dmabuf_binding struct is refcounted, and releases its > resources only when all the refs are released. > > [1] https://lore.kernel.org/netdev/20230707183935.997267-1-kuba@kernel.org/ > > Signed-off-by: Willem de Bruijn <willemb@google.com> > Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com> > > Signed-off-by: Mina Almasry <almasrymina@google.com> > --- > include/linux/netdevice.h | 57 ++++++++++++ > include/net/page_pool.h | 27 ++++++ > net/core/dev.c | 178 ++++++++++++++++++++++++++++++++++++++ > net/core/netdev-genl.c | 101 ++++++++++++++++++++- > 4 files changed, 361 insertions(+), 2 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 3800d0479698..1b7c5966d2ca 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -53,6 +53,8 @@ > #include <net/net_trackers.h> > #include <net/net_debug.h> > #include <net/dropreason-core.h> > +#include <linux/xarray.h> > +#include <linux/refcount.h> > > struct netpoll_info; > struct device; > @@ -782,6 +784,55 @@ bool rps_may_expire_flow(struct net_device *dev, u16 rxq_index, u32 flow_id, > #endif > #endif /* CONFIG_RPS */ > > +struct netdev_dmabuf_binding { > + struct dma_buf *dmabuf; > + struct dma_buf_attachment *attachment; > + struct sg_table *sgt; > + struct net_device *dev; > + struct gen_pool *chunk_pool; > + > + /* The user holds a ref (via the netlink API) for as long as they want > + * the binding to remain alive. Each page pool using this binding holds > + * a ref to keep the binding alive. Each allocated page_pool_iov holds a > + * ref. > + * > + * The binding undos itself and unmaps the underlying dmabuf once all > + * those refs are dropped and the binding is no longer desired or in > + * use. > + */ > + refcount_t ref; > + > + /* The portid of the user that owns this binding. Used for netlink to > + * notify us of the user dropping the bind. > + */ > + u32 owner_nlportid; > + > + /* The list of bindings currently active. Used for netlink to notify us > + * of the user dropping the bind. > + */ > + struct list_head list; > + > + /* rxq's this binding is active on. */ > + struct xarray bound_rxq_list; > +}; > + > +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding); > + > +static inline void > +netdev_devmem_binding_get(struct netdev_dmabuf_binding *binding) > +{ > + refcount_inc(&binding->ref); > +} > + > +static inline void > +netdev_devmem_binding_put(struct netdev_dmabuf_binding *binding) > +{ > + if (!refcount_dec_and_test(&binding->ref)) > + return; > + > + __netdev_devmem_binding_free(binding); > +} > + > /* This structure contains an instance of an RX queue. */ > struct netdev_rx_queue { > struct xdp_rxq_info xdp_rxq; > @@ -796,6 +847,7 @@ struct netdev_rx_queue { > #ifdef CONFIG_XDP_SOCKETS > struct xsk_buff_pool *pool; > #endif > + struct netdev_dmabuf_binding *binding; > } ____cacheline_aligned_in_smp; > > /* > @@ -5026,6 +5078,11 @@ void netif_set_tso_max_segs(struct net_device *dev, unsigned int segs); > void netif_inherit_tso_max(struct net_device *to, > const struct net_device *from); > > +void netdev_unbind_dmabuf_to_queue(struct netdev_dmabuf_binding *binding); > +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int dmabuf_fd, > + u32 rxq_idx, > + struct netdev_dmabuf_binding **out); > + > static inline bool netif_is_macsec(const struct net_device *dev) > { > return dev->priv_flags & IFF_MACSEC; > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index 364fe6924258..61b2066d32b5 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -170,6 +170,33 @@ extern const struct pp_memory_provider_ops hugesp_ops; > extern const struct pp_memory_provider_ops huge_ops; > extern const struct pp_memory_provider_ops huge_1g_ops; > > +/* page_pool_iov support */ > + > +/* Owner of the dma-buf chunks inserted into the gen pool. Each scatterlist > + * entry from the dmabuf is inserted into the genpool as a chunk, and needs > + * this owner struct to keep track of some metadata necessary to create > + * allocations from this chunk. > + */ > +struct dmabuf_genpool_chunk_owner { > + /* Offset into the dma-buf where this chunk starts. */ > + unsigned long base_virtual; > + > + /* dma_addr of the start of the chunk. */ > + dma_addr_t base_dma_addr; > + > + /* Array of page_pool_iovs for this chunk. */ > + struct page_pool_iov *ppiovs; > + size_t num_ppiovs; > + > + struct netdev_dmabuf_binding *binding; > +}; > + > +struct page_pool_iov { > + struct dmabuf_genpool_chunk_owner *owner; > + > + refcount_t refcount; > +}; > + Hi Mina, we're working on ZC RX to host memory [1] and have a similar need for a page pool memory provider (backed by userspace host memory instead of GPU memory) that hands out generic page_pool_iov buffers. The current differences are that we hold a page ptr and its dma_addr_t directly as we are backed by real pages; we still need a refcount for lifetime management. See struct io_zc_rx_buf in [2]. It would be great to align on page_pool_iov such that it will work for both of us. Would a union work here? [1] https://lore.kernel.org/io-uring/20230826011954.1801099-1-dw@davidwei.uk/ [2] https://lore.kernel.org/io-uring/20230826011954.1801099-6-dw@davidwei.uk/ > struct page_pool { > struct page_pool_params p; > > diff --git a/net/core/dev.c b/net/core/dev.c > index 8e7d0cb540cd..02a25ccf771a 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -151,6 +151,8 @@ > #include <linux/pm_runtime.h> > #include <linux/prandom.h> > #include <linux/once_lite.h> > +#include <linux/genalloc.h> > +#include <linux/dma-buf.h> > > #include "dev.h" > #include "net-sysfs.h" > @@ -2037,6 +2039,182 @@ static int call_netdevice_notifiers_mtu(unsigned long val, > return call_netdevice_notifiers_info(val, &info.info); > } > > +/* Device memory support */ > + > +static void netdev_devmem_free_chunk_owner(struct gen_pool *genpool, > + struct gen_pool_chunk *chunk, > + void *not_used) > +{ > + struct dmabuf_genpool_chunk_owner *owner = chunk->owner; > + > + kvfree(owner->ppiovs); > + kfree(owner); > +} > + > +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding) > +{ > + size_t size, avail; > + > + gen_pool_for_each_chunk(binding->chunk_pool, > + netdev_devmem_free_chunk_owner, NULL); > + > + size = gen_pool_size(binding->chunk_pool); > + avail = gen_pool_avail(binding->chunk_pool); > + > + if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu", > + size, avail)) > + gen_pool_destroy(binding->chunk_pool); > + > + dma_buf_unmap_attachment(binding->attachment, binding->sgt, > + DMA_BIDIRECTIONAL); > + dma_buf_detach(binding->dmabuf, binding->attachment); > + dma_buf_put(binding->dmabuf); > + kfree(binding); > +} > + > +void netdev_unbind_dmabuf_to_queue(struct netdev_dmabuf_binding *binding) > +{ > + struct netdev_rx_queue *rxq; > + unsigned long xa_idx; > + > + list_del_rcu(&binding->list); > + > + xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) > + if (rxq->binding == binding) > + rxq->binding = NULL; > + > + netdev_devmem_binding_put(binding); > +} > + > +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int dmabuf_fd, > + u32 rxq_idx, struct netdev_dmabuf_binding **out) > +{ > + struct netdev_dmabuf_binding *binding; > + struct netdev_rx_queue *rxq; > + struct scatterlist *sg; > + struct dma_buf *dmabuf; > + unsigned int sg_idx, i; > + unsigned long virtual; > + u32 xa_idx; > + int err; > + > + rxq = __netif_get_rx_queue(dev, rxq_idx); > + > + if (rxq->binding) > + return -EEXIST; > + > + dmabuf = dma_buf_get(dmabuf_fd); > + if (IS_ERR_OR_NULL(dmabuf)) > + return -EBADFD; > + > + binding = kzalloc_node(sizeof(*binding), GFP_KERNEL, > + dev_to_node(&dev->dev)); > + if (!binding) { > + err = -ENOMEM; > + goto err_put_dmabuf; > + } > + > + xa_init_flags(&binding->bound_rxq_list, XA_FLAGS_ALLOC); > + > + refcount_set(&binding->ref, 1); > + > + binding->dmabuf = dmabuf; > + > + binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent); > + if (IS_ERR(binding->attachment)) { > + err = PTR_ERR(binding->attachment); > + goto err_free_binding; > + } > + > + binding->sgt = dma_buf_map_attachment(binding->attachment, > + DMA_BIDIRECTIONAL); > + if (IS_ERR(binding->sgt)) { > + err = PTR_ERR(binding->sgt); > + goto err_detach; > + } > + > + /* 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)); > + if (!binding->chunk_pool) { > + err = -ENOMEM; > + goto err_unmap; > + } > + > + virtual = 0; > + for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) { > + dma_addr_t dma_addr = sg_dma_address(sg); > + struct dmabuf_genpool_chunk_owner *owner; > + size_t len = sg_dma_len(sg); > + struct page_pool_iov *ppiov; > + > + owner = kzalloc_node(sizeof(*owner), GFP_KERNEL, > + dev_to_node(&dev->dev)); > + owner->base_virtual = virtual; > + owner->base_dma_addr = dma_addr; > + owner->num_ppiovs = len / PAGE_SIZE; > + owner->binding = binding; > + > + err = gen_pool_add_owner(binding->chunk_pool, dma_addr, > + dma_addr, len, dev_to_node(&dev->dev), > + owner); > + if (err) { > + err = -EINVAL; > + goto err_free_chunks; > + } > + > + owner->ppiovs = kvmalloc_array(owner->num_ppiovs, > + sizeof(*owner->ppiovs), > + GFP_KERNEL); > + if (!owner->ppiovs) { > + err = -ENOMEM; > + goto err_free_chunks; > + } > + > + for (i = 0; i < owner->num_ppiovs; i++) { > + ppiov = &owner->ppiovs[i]; > + ppiov->owner = owner; > + refcount_set(&ppiov->refcount, 1); > + } > + > + dma_addr += len; > + virtual += len; > + } > + > + /* TODO: need to be able to bind to multiple rx queues on the same > + * netdevice. The code should already be able to handle that with > + * minimal changes, but the netlink API currently allows for 1 rx > + * queue. > + */ > + err = xa_alloc(&binding->bound_rxq_list, &xa_idx, rxq, xa_limit_32b, > + GFP_KERNEL); > + if (err) > + goto err_free_chunks; > + > + rxq->binding = binding; > + *out = binding; > + > + return 0; > + > +err_free_chunks: > + gen_pool_for_each_chunk(binding->chunk_pool, > + netdev_devmem_free_chunk_owner, NULL); > + gen_pool_destroy(binding->chunk_pool); > +err_unmap: > + dma_buf_unmap_attachment(binding->attachment, binding->sgt, > + DMA_BIDIRECTIONAL); > +err_detach: > + dma_buf_detach(dmabuf, binding->attachment); > +err_free_binding: > + kfree(binding); > +err_put_dmabuf: > + dma_buf_put(dmabuf); > + return err; > +} > + > #ifdef CONFIG_NET_INGRESS > static DEFINE_STATIC_KEY_FALSE(ingress_needed_key); > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > index bf7324dd6c36..288ed0112995 100644 > --- a/net/core/netdev-genl.c > +++ b/net/core/netdev-genl.c > @@ -141,10 +141,74 @@ int netdev_nl_dev_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) > return skb->len; > } > > -/* Stub */ > +static LIST_HEAD(netdev_rbinding_list); > + > int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info) > { > - return 0; > + struct netdev_dmabuf_binding *out_binding; > + u32 ifindex, dmabuf_fd, rxq_idx; > + struct net_device *netdev; > + struct sk_buff *rsp; > + int err = 0; > + void *hdr; > + > + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_DEV_IFINDEX) || > + GENL_REQ_ATTR_CHECK(info, NETDEV_A_BIND_DMABUF_DMABUF_FD) || > + GENL_REQ_ATTR_CHECK(info, NETDEV_A_BIND_DMABUF_QUEUE_IDX)) > + return -EINVAL; > + > + ifindex = nla_get_u32(info->attrs[NETDEV_A_DEV_IFINDEX]); > + dmabuf_fd = nla_get_u32(info->attrs[NETDEV_A_BIND_DMABUF_DMABUF_FD]); > + rxq_idx = nla_get_u32(info->attrs[NETDEV_A_BIND_DMABUF_QUEUE_IDX]); > + > + rtnl_lock(); > + > + netdev = __dev_get_by_index(genl_info_net(info), ifindex); > + if (!netdev) { > + err = -ENODEV; > + goto err_unlock; > + } > + > + if (rxq_idx >= netdev->num_rx_queues) { > + err = -ERANGE; > + goto err_unlock; > + } > + > + if (netdev_bind_dmabuf_to_queue(netdev, dmabuf_fd, rxq_idx, > + &out_binding)) { > + err = -EINVAL; > + goto err_unlock; > + } > + > + out_binding->owner_nlportid = info->snd_portid; > + list_add_rcu(&out_binding->list, &netdev_rbinding_list); > + > + rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!rsp) { > + err = -ENOMEM; > + goto err_unbind; > + } > + > + hdr = genlmsg_put(rsp, info->snd_portid, info->snd_seq, > + &netdev_nl_family, 0, info->genlhdr->cmd); > + if (!hdr) { > + err = -EMSGSIZE; > + goto err_genlmsg_free; > + } > + > + genlmsg_end(rsp, hdr); > + > + rtnl_unlock(); > + > + return genlmsg_reply(rsp, info); > + > +err_genlmsg_free: > + nlmsg_free(rsp); > +err_unbind: > + netdev_unbind_dmabuf_to_queue(out_binding); > +err_unlock: > + rtnl_unlock(); > + return err; > } > > static int netdev_genl_netdevice_event(struct notifier_block *nb, > @@ -167,10 +231,37 @@ static int netdev_genl_netdevice_event(struct notifier_block *nb, > return NOTIFY_OK; > } > > +static int netdev_netlink_notify(struct notifier_block *nb, unsigned long state, > + void *_notify) > +{ > + struct netlink_notify *notify = _notify; > + struct netdev_dmabuf_binding *rbinding; > + > + if (state != NETLINK_URELEASE || notify->protocol != NETLINK_GENERIC) > + return NOTIFY_DONE; > + > + rcu_read_lock(); > + > + list_for_each_entry_rcu(rbinding, &netdev_rbinding_list, list) { > + if (rbinding->owner_nlportid == notify->portid) { > + netdev_unbind_dmabuf_to_queue(rbinding); > + break; > + } > + } > + > + rcu_read_unlock(); > + > + return NOTIFY_OK; > +} > + > static struct notifier_block netdev_genl_nb = { > .notifier_call = netdev_genl_netdevice_event, > }; > > +static struct notifier_block netdev_netlink_notifier = { > + .notifier_call = netdev_netlink_notify, > +}; > + > static int __init netdev_genl_init(void) > { > int err; > @@ -183,8 +274,14 @@ static int __init netdev_genl_init(void) > if (err) > goto err_unreg_ntf; > > + err = netlink_register_notifier(&netdev_netlink_notifier); > + if (err) > + goto err_unreg_family; > + > return 0; > > +err_unreg_family: > + genl_unregister_family(&netdev_nl_family); > err_unreg_ntf: > unregister_netdevice_notifier(&netdev_genl_nb); > return err; > -- > 2.41.0.640.ga95def55d0-goog
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 3800d0479698..1b7c5966d2ca 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -53,6 +53,8 @@ #include <net/net_trackers.h> #include <net/net_debug.h> #include <net/dropreason-core.h> +#include <linux/xarray.h> +#include <linux/refcount.h> struct netpoll_info; struct device; @@ -782,6 +784,55 @@ bool rps_may_expire_flow(struct net_device *dev, u16 rxq_index, u32 flow_id, #endif #endif /* CONFIG_RPS */ +struct netdev_dmabuf_binding { + struct dma_buf *dmabuf; + struct dma_buf_attachment *attachment; + struct sg_table *sgt; + struct net_device *dev; + struct gen_pool *chunk_pool; + + /* The user holds a ref (via the netlink API) for as long as they want + * the binding to remain alive. Each page pool using this binding holds + * a ref to keep the binding alive. Each allocated page_pool_iov holds a + * ref. + * + * The binding undos itself and unmaps the underlying dmabuf once all + * those refs are dropped and the binding is no longer desired or in + * use. + */ + refcount_t ref; + + /* The portid of the user that owns this binding. Used for netlink to + * notify us of the user dropping the bind. + */ + u32 owner_nlportid; + + /* The list of bindings currently active. Used for netlink to notify us + * of the user dropping the bind. + */ + struct list_head list; + + /* rxq's this binding is active on. */ + struct xarray bound_rxq_list; +}; + +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding); + +static inline void +netdev_devmem_binding_get(struct netdev_dmabuf_binding *binding) +{ + refcount_inc(&binding->ref); +} + +static inline void +netdev_devmem_binding_put(struct netdev_dmabuf_binding *binding) +{ + if (!refcount_dec_and_test(&binding->ref)) + return; + + __netdev_devmem_binding_free(binding); +} + /* This structure contains an instance of an RX queue. */ struct netdev_rx_queue { struct xdp_rxq_info xdp_rxq; @@ -796,6 +847,7 @@ struct netdev_rx_queue { #ifdef CONFIG_XDP_SOCKETS struct xsk_buff_pool *pool; #endif + struct netdev_dmabuf_binding *binding; } ____cacheline_aligned_in_smp; /* @@ -5026,6 +5078,11 @@ void netif_set_tso_max_segs(struct net_device *dev, unsigned int segs); void netif_inherit_tso_max(struct net_device *to, const struct net_device *from); +void netdev_unbind_dmabuf_to_queue(struct netdev_dmabuf_binding *binding); +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int dmabuf_fd, + u32 rxq_idx, + struct netdev_dmabuf_binding **out); + static inline bool netif_is_macsec(const struct net_device *dev) { return dev->priv_flags & IFF_MACSEC; diff --git a/include/net/page_pool.h b/include/net/page_pool.h index 364fe6924258..61b2066d32b5 100644 --- a/include/net/page_pool.h +++ b/include/net/page_pool.h @@ -170,6 +170,33 @@ extern const struct pp_memory_provider_ops hugesp_ops; extern const struct pp_memory_provider_ops huge_ops; extern const struct pp_memory_provider_ops huge_1g_ops; +/* page_pool_iov support */ + +/* Owner of the dma-buf chunks inserted into the gen pool. Each scatterlist + * entry from the dmabuf is inserted into the genpool as a chunk, and needs + * this owner struct to keep track of some metadata necessary to create + * allocations from this chunk. + */ +struct dmabuf_genpool_chunk_owner { + /* Offset into the dma-buf where this chunk starts. */ + unsigned long base_virtual; + + /* dma_addr of the start of the chunk. */ + dma_addr_t base_dma_addr; + + /* Array of page_pool_iovs for this chunk. */ + struct page_pool_iov *ppiovs; + size_t num_ppiovs; + + struct netdev_dmabuf_binding *binding; +}; + +struct page_pool_iov { + struct dmabuf_genpool_chunk_owner *owner; + + refcount_t refcount; +}; + struct page_pool { struct page_pool_params p; diff --git a/net/core/dev.c b/net/core/dev.c index 8e7d0cb540cd..02a25ccf771a 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -151,6 +151,8 @@ #include <linux/pm_runtime.h> #include <linux/prandom.h> #include <linux/once_lite.h> +#include <linux/genalloc.h> +#include <linux/dma-buf.h> #include "dev.h" #include "net-sysfs.h" @@ -2037,6 +2039,182 @@ static int call_netdevice_notifiers_mtu(unsigned long val, return call_netdevice_notifiers_info(val, &info.info); } +/* Device memory support */ + +static void netdev_devmem_free_chunk_owner(struct gen_pool *genpool, + struct gen_pool_chunk *chunk, + void *not_used) +{ + struct dmabuf_genpool_chunk_owner *owner = chunk->owner; + + kvfree(owner->ppiovs); + kfree(owner); +} + +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding) +{ + size_t size, avail; + + gen_pool_for_each_chunk(binding->chunk_pool, + netdev_devmem_free_chunk_owner, NULL); + + size = gen_pool_size(binding->chunk_pool); + avail = gen_pool_avail(binding->chunk_pool); + + if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu", + size, avail)) + gen_pool_destroy(binding->chunk_pool); + + dma_buf_unmap_attachment(binding->attachment, binding->sgt, + DMA_BIDIRECTIONAL); + dma_buf_detach(binding->dmabuf, binding->attachment); + dma_buf_put(binding->dmabuf); + kfree(binding); +} + +void netdev_unbind_dmabuf_to_queue(struct netdev_dmabuf_binding *binding) +{ + struct netdev_rx_queue *rxq; + unsigned long xa_idx; + + list_del_rcu(&binding->list); + + xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) + if (rxq->binding == binding) + rxq->binding = NULL; + + netdev_devmem_binding_put(binding); +} + +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int dmabuf_fd, + u32 rxq_idx, struct netdev_dmabuf_binding **out) +{ + struct netdev_dmabuf_binding *binding; + struct netdev_rx_queue *rxq; + struct scatterlist *sg; + struct dma_buf *dmabuf; + unsigned int sg_idx, i; + unsigned long virtual; + u32 xa_idx; + int err; + + rxq = __netif_get_rx_queue(dev, rxq_idx); + + if (rxq->binding) + return -EEXIST; + + dmabuf = dma_buf_get(dmabuf_fd); + if (IS_ERR_OR_NULL(dmabuf)) + return -EBADFD; + + binding = kzalloc_node(sizeof(*binding), GFP_KERNEL, + dev_to_node(&dev->dev)); + if (!binding) { + err = -ENOMEM; + goto err_put_dmabuf; + } + + xa_init_flags(&binding->bound_rxq_list, XA_FLAGS_ALLOC); + + refcount_set(&binding->ref, 1); + + binding->dmabuf = dmabuf; + + binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent); + if (IS_ERR(binding->attachment)) { + err = PTR_ERR(binding->attachment); + goto err_free_binding; + } + + binding->sgt = dma_buf_map_attachment(binding->attachment, + DMA_BIDIRECTIONAL); + if (IS_ERR(binding->sgt)) { + err = PTR_ERR(binding->sgt); + goto err_detach; + } + + /* 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)); + if (!binding->chunk_pool) { + err = -ENOMEM; + goto err_unmap; + } + + virtual = 0; + for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) { + dma_addr_t dma_addr = sg_dma_address(sg); + struct dmabuf_genpool_chunk_owner *owner; + size_t len = sg_dma_len(sg); + struct page_pool_iov *ppiov; + + owner = kzalloc_node(sizeof(*owner), GFP_KERNEL, + dev_to_node(&dev->dev)); + owner->base_virtual = virtual; + owner->base_dma_addr = dma_addr; + owner->num_ppiovs = len / PAGE_SIZE; + owner->binding = binding; + + err = gen_pool_add_owner(binding->chunk_pool, dma_addr, + dma_addr, len, dev_to_node(&dev->dev), + owner); + if (err) { + err = -EINVAL; + goto err_free_chunks; + } + + owner->ppiovs = kvmalloc_array(owner->num_ppiovs, + sizeof(*owner->ppiovs), + GFP_KERNEL); + if (!owner->ppiovs) { + err = -ENOMEM; + goto err_free_chunks; + } + + for (i = 0; i < owner->num_ppiovs; i++) { + ppiov = &owner->ppiovs[i]; + ppiov->owner = owner; + refcount_set(&ppiov->refcount, 1); + } + + dma_addr += len; + virtual += len; + } + + /* TODO: need to be able to bind to multiple rx queues on the same + * netdevice. The code should already be able to handle that with + * minimal changes, but the netlink API currently allows for 1 rx + * queue. + */ + err = xa_alloc(&binding->bound_rxq_list, &xa_idx, rxq, xa_limit_32b, + GFP_KERNEL); + if (err) + goto err_free_chunks; + + rxq->binding = binding; + *out = binding; + + return 0; + +err_free_chunks: + gen_pool_for_each_chunk(binding->chunk_pool, + netdev_devmem_free_chunk_owner, NULL); + gen_pool_destroy(binding->chunk_pool); +err_unmap: + dma_buf_unmap_attachment(binding->attachment, binding->sgt, + DMA_BIDIRECTIONAL); +err_detach: + dma_buf_detach(dmabuf, binding->attachment); +err_free_binding: + kfree(binding); +err_put_dmabuf: + dma_buf_put(dmabuf); + return err; +} + #ifdef CONFIG_NET_INGRESS static DEFINE_STATIC_KEY_FALSE(ingress_needed_key); diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c index bf7324dd6c36..288ed0112995 100644 --- a/net/core/netdev-genl.c +++ b/net/core/netdev-genl.c @@ -141,10 +141,74 @@ int netdev_nl_dev_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) return skb->len; } -/* Stub */ +static LIST_HEAD(netdev_rbinding_list); + int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info) { - return 0; + struct netdev_dmabuf_binding *out_binding; + u32 ifindex, dmabuf_fd, rxq_idx; + struct net_device *netdev; + struct sk_buff *rsp; + int err = 0; + void *hdr; + + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_DEV_IFINDEX) || + GENL_REQ_ATTR_CHECK(info, NETDEV_A_BIND_DMABUF_DMABUF_FD) || + GENL_REQ_ATTR_CHECK(info, NETDEV_A_BIND_DMABUF_QUEUE_IDX)) + return -EINVAL; + + ifindex = nla_get_u32(info->attrs[NETDEV_A_DEV_IFINDEX]); + dmabuf_fd = nla_get_u32(info->attrs[NETDEV_A_BIND_DMABUF_DMABUF_FD]); + rxq_idx = nla_get_u32(info->attrs[NETDEV_A_BIND_DMABUF_QUEUE_IDX]); + + rtnl_lock(); + + netdev = __dev_get_by_index(genl_info_net(info), ifindex); + if (!netdev) { + err = -ENODEV; + goto err_unlock; + } + + if (rxq_idx >= netdev->num_rx_queues) { + err = -ERANGE; + goto err_unlock; + } + + if (netdev_bind_dmabuf_to_queue(netdev, dmabuf_fd, rxq_idx, + &out_binding)) { + err = -EINVAL; + goto err_unlock; + } + + out_binding->owner_nlportid = info->snd_portid; + list_add_rcu(&out_binding->list, &netdev_rbinding_list); + + rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!rsp) { + err = -ENOMEM; + goto err_unbind; + } + + hdr = genlmsg_put(rsp, info->snd_portid, info->snd_seq, + &netdev_nl_family, 0, info->genlhdr->cmd); + if (!hdr) { + err = -EMSGSIZE; + goto err_genlmsg_free; + } + + genlmsg_end(rsp, hdr); + + rtnl_unlock(); + + return genlmsg_reply(rsp, info); + +err_genlmsg_free: + nlmsg_free(rsp); +err_unbind: + netdev_unbind_dmabuf_to_queue(out_binding); +err_unlock: + rtnl_unlock(); + return err; } static int netdev_genl_netdevice_event(struct notifier_block *nb, @@ -167,10 +231,37 @@ static int netdev_genl_netdevice_event(struct notifier_block *nb, return NOTIFY_OK; } +static int netdev_netlink_notify(struct notifier_block *nb, unsigned long state, + void *_notify) +{ + struct netlink_notify *notify = _notify; + struct netdev_dmabuf_binding *rbinding; + + if (state != NETLINK_URELEASE || notify->protocol != NETLINK_GENERIC) + return NOTIFY_DONE; + + rcu_read_lock(); + + list_for_each_entry_rcu(rbinding, &netdev_rbinding_list, list) { + if (rbinding->owner_nlportid == notify->portid) { + netdev_unbind_dmabuf_to_queue(rbinding); + break; + } + } + + rcu_read_unlock(); + + return NOTIFY_OK; +} + static struct notifier_block netdev_genl_nb = { .notifier_call = netdev_genl_netdevice_event, }; +static struct notifier_block netdev_netlink_notifier = { + .notifier_call = netdev_netlink_notify, +}; + static int __init netdev_genl_init(void) { int err; @@ -183,8 +274,14 @@ static int __init netdev_genl_init(void) if (err) goto err_unreg_ntf; + err = netlink_register_notifier(&netdev_netlink_notifier); + if (err) + goto err_unreg_family; + return 0; +err_unreg_family: + genl_unregister_family(&netdev_nl_family); err_unreg_ntf: unregister_netdevice_notifier(&netdev_genl_nb); return err;