On 2023/11/6 10:44, Mina Almasry wrote:

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h index 6fc5134095ed..d4bea053bb7e 100644 --- a/include/net/page_pool/types.h +++ b/include/net/page_pool/types.h @@ -60,6 +60,8 @@ struct page_pool_params { int nid; struct device *dev; struct napi_struct *napi;

• u8 memory_provider;
• void *mp_priv; enum dma_data_direction dma_dir; unsigned int max_len; unsigned int offset;

@@ -118,6 +120,19 @@ struct page_pool_stats { }; #endif +struct mem_provider;

The above doesn't seems be used?

+enum pp_memory_provider_type {

• __PP_MP_NONE, /* Use system allocator directly */

+};

+struct pp_memory_provider_ops {

Is it better to rename the above to pp_memory_provider and put the above memory_provider and mp_priv here? so that all the fields related to pp_memory_provider are in one place?

It is probably better to provide a register function for driver to implement its own pp_memory_provider in the future.

• int (*init)(struct page_pool *pool);
• void (*destroy)(struct page_pool *pool);
• struct page *(*alloc_pages)(struct page_pool *pool, gfp_t gfp);
• bool (*release_page)(struct page_pool *pool, struct page *page);

+};

On Sun, 5 Nov 2023 18:44:01 -0800 Mina Almasry wrote:

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h index 6fc5134095ed..d4bea053bb7e 100644 --- a/include/net/page_pool/types.h +++ b/include/net/page_pool/types.h @@ -60,6 +60,8 @@ struct page_pool_params { int nid; struct device *dev; struct napi_struct *napi;

• u8 memory_provider;
• void *mp_priv; enum dma_data_direction dma_dir; unsigned int max_len; unsigned int offset;

you should rebase on top of net-next

More importantly I was expecting those fields to be gone from params. The fact that the page pool is configured to a specific provider should be fully transparent to the driver, driver should just tell the core what queue its creating the pool from and if there's a dmabuf bound for that queue - out pops a pp backed by the dmabuf.

My RFC had the page pool params fields here as a hack.

On Wed, Nov 8, 2023 at 3:47 PM David Wei dw@davidwei.uk wrote:

On 2023-11-05 18:44, 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 rx queues 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.

The netdev_dmabuf_binding struct is refcounted, and releases its resources only when all the refs are released.

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

---

RFC v3:

• Support multi rx-queue binding

---

include/linux/netdevice.h | 80 ++++++++++++++ include/net/netdev_rx_queue.h | 1 + include/net/page_pool/types.h | 27 +++++ net/core/dev.c | 203 ++++++++++++++++++++++++++++++++++ net/core/netdev-genl.c | 116 ++++++++++++++++++- 5 files changed, 425 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index b8bf669212cc..eeeda849115c 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -52,6 +52,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; @@ -808,6 +810,84 @@ 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;
  

+};

+#ifdef CONFIG_DMA_SHARED_BUFFER +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding); +int netdev_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,

•                struct netdev_dmabuf_binding **out);
  

+void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding); +int netdev_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,

•                         struct netdev_dmabuf_binding *binding);
  

+#else +static inline void +__netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding) +{ +}

+static inline int netdev_bind_dmabuf(struct net_device *dev,

•                              unsigned int dmabuf_fd,
  

•                              struct netdev_dmabuf_binding **out)
  

+{

• return -EOPNOTSUPP;
  

+} +static inline void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding) +{ +}

+static inline int +netdev_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,

•                     struct netdev_dmabuf_binding *binding)
  

+{

• return -EOPNOTSUPP;
  

+} +#endif

+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);
  

+}

/* XPS map type and offset of the xps map within net_device->xps_maps[]. */ enum xps_map_type { XPS_CPUS = 0, diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h index cdcafb30d437..1bfcf60a145d 100644 --- a/include/net/netdev_rx_queue.h +++ b/include/net/netdev_rx_queue.h @@ -21,6 +21,7 @@ struct netdev_rx_queue { #ifdef CONFIG_XDP_SOCKETS struct xsk_buff_pool *pool; #endif

• struct netdev_dmabuf_binding *binding;
  

@Pavel - They are using struct netdev_rx_queue to hold the binding, which is an object that holds the state and is mapped 1:1 to an rxq. This object is similar to our "interface queue". I wonder if we should re-visit using this generic struct, instead of driver specific structs e.g. bnxt_rx_ring_info?

...

} ____cacheline_aligned_in_smp;

/* diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h index d4bea053bb7e..64386325d965 100644 --- a/include/net/page_pool/types.h +++ b/include/net/page_pool/types.h @@ -133,6 +133,33 @@ struct pp_memory_provider_ops { bool (*release_page)(struct page_pool *pool, struct page *page); };

+/* 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 a37a932a3e14..c8c3709d42c8 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -153,6 +153,9 @@ #include <linux/prandom.h> #include <linux/once_lite.h> #include <net/netdev_rx_queue.h> +#include <linux/genalloc.h> +#include <linux/dma-buf.h> +#include <net/page_pool/types.h>

#include "dev.h" #include "net-sysfs.h" @@ -2040,6 +2043,206 @@ static int call_netdevice_notifiers_mtu(unsigned long val, return call_netdevice_notifiers_info(val, &info.info); }

+/* Device memory support */

+#ifdef CONFIG_DMA_SHARED_BUFFER +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(struct netdev_dmabuf_binding *binding) +{

• struct netdev_rx_queue *rxq;
  

• unsigned long xa_idx;
  

• if (!binding)
  

•         return;
  

• list_del_rcu(&binding->list);
  

• xa_for_each(&binding->bound_rxq_list, xa_idx, rxq)
  

•         if (rxq->binding == binding)
  

•                 /* We hold the rtnl_lock while binding/unbinding
  

•                  * dma-buf, so we can't race with another thread that
  

•                  * is also modifying this value. However, the driver
  

•                  * may read this config while it's creating its
  

•                  * rx-queues. WRITE_ONCE() here to match the
  

•                  * READ_ONCE() in the driver.
  

•                  */
  

•                 WRITE_ONCE(rxq->binding, NULL);
  

• netdev_devmem_binding_put(binding);
  

+}

+int netdev_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,

•                         struct netdev_dmabuf_binding *binding)
  

+{

• struct netdev_rx_queue *rxq;
  

• u32 xa_idx;
  

• int err;
  

• rxq = __netif_get_rx_queue(dev, rxq_idx);
  

• if (rxq->binding)
  

•         return -EEXIST;
  

• err = xa_alloc(&binding->bound_rxq_list, &xa_idx, rxq, xa_limit_32b,
  

•                GFP_KERNEL);
  

• if (err)
  

•         return err;
  

• /*We hold the rtnl_lock while binding/unbinding dma-buf, so we can't
  

•  * race with another thread that is also modifying this value. However,
  

•  * the driver may read this config while it's creating its * rx-queues.
  

•  * WRITE_ONCE() here to match the READ_ONCE() in the driver.
  

•  */
  

• WRITE_ONCE(rxq->binding, binding);
  

• return 0;
  

+}

+int netdev_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,

•                struct netdev_dmabuf_binding **out)
  

I'm not entirely familiar with the Netlink API. Mina, do you know if we can call into netdev_bind_dmabuf or netdev_nl_bind_rx_doit directly, without needing to call send/recv on a Netlink socket? We likely want io_uring to do the registration of a dmabuf fd and keep ownership over it.

You can likely call into netdev_bind_dmabuf(), but not netdev_nl_bind_rx_doit. The latter is very netlink specific.

...

+{

• struct netdev_dmabuf_binding *binding;
  

• struct scatterlist *sg;
  

• struct dma_buf *dmabuf;
  

• unsigned int sg_idx, i;
  

• unsigned long virtual;
  

• int err;
  

• if (!capable(CAP_NET_ADMIN))
  

•         return -EPERM;
  

• 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;
  

• }
  

• *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;
  

+} +#endif

#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 59d3d512d9cc..2c2a62593217 100644 --- a/net/core/netdev-genl.c +++ b/net/core/netdev-genl.c @@ -129,10 +129,89 @@ 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 rem, err = 0;
  

• void *hdr;
  

• struct nlattr *attr;
  

• 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_QUEUES))
  

•         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]);
  

• rtnl_lock();
  

• netdev = __dev_get_by_index(genl_info_net(info), ifindex);
  

• if (!netdev) {
  

•         err = -ENODEV;
  

•         goto err_unlock;
  

• }
  

• err = netdev_bind_dmabuf(netdev, dmabuf_fd, &out_binding);
  

• if (err)
  

•         goto err_unlock;
  

• nla_for_each_attr(attr, genlmsg_data(info->genlhdr),
  

•                   genlmsg_len(info->genlhdr), rem) {
  

•         switch (nla_type(attr)) {
  

•         case NETDEV_A_BIND_DMABUF_QUEUES:
  

•                 rxq_idx = nla_get_u32(attr);
  

•                 if (rxq_idx >= netdev->num_rx_queues) {
  

•                         err = -ERANGE;
  

•                         goto err_unbind;
  

•                 }
  

•                 err = netdev_bind_dmabuf_to_queue(netdev, rxq_idx,
  

•                                                   out_binding);
  

•                 if (err)
  

•                         goto err_unbind;
  

•                 break;
  

•         default:
  

•                 break;
  

•         }
  

• }
  

• 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(out_binding);
  

+err_unlock:

• rtnl_unlock();
  

• return err;
  

}

static int netdev_genl_netdevice_event(struct notifier_block *nb, @@ -155,10 +234,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(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,
  

+};

Is this mechamism what cleans up TCP devmem in case userspace crashes and the associated Netlink socket is closed?

Correct.

...

static int __init netdev_genl_init(void) { int err; @@ -171,8 +277,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;

On Thu, Nov 9, 2023 at 11:38 PM Yunsheng Lin linyunsheng@huawei.com wrote:

On 2023/11/10 10:59, Mina Almasry wrote:

...

On Thu, Nov 9, 2023 at 12:30 AM Paolo Abeni pabeni@redhat.com wrote:

...

I'm trying to wrap my head around the whole infra... the above line is confusing. Why do you increment dma_addr? it will be re-initialized in the next iteration.

That is just a mistake, sorry. Will remove this increment.

You seems to be combining comments in different thread and replying in one thread, I am not sure that is a good practice and I almost missed the reply below as I don't seem to be cc'ed.

Sorry about that.

...

On Thu, Nov 9, 2023 at 1:29 AM Yunsheng Lin linyunsheng@huawei.com wrote:> >>>

...

...

...

...

gen_pool_destroy BUG_ON() if it's not empty at the time of destroying. Technically that should never happen, because __netdev_devmem_binding_free() should only be called when the refcount hits 0, so all the chunks have been freed back to the gen_pool. But, just in case, I don't want to crash the server just because I'm leaking a chunk... this is a bit of defensive programming that is typically frowned upon, but the behavior of gen_pool is so severe I think the WARN() + check is warranted here.

It seems it is pretty normal for the above to happen nowadays because of retransmits timeouts, NAPI defer schemes mentioned below:

https://lkml.kernel.org/netdev/168269854650.2191653.8465259808498269815.stgi...

And currently page pool core handles that by using a workqueue.

Forgive me but I'm not understanding the concern here.

__netdev_devmem_binding_free() is called when binding->ref hits 0.

binding->ref is incremented when an iov slice of the dma-buf is allocated, and decremented when an iov is freed. So, __netdev_devmem_binding_free() can't really be called unless all the iovs have been freed, and gen_pool_size() == gen_pool_avail(), regardless of what's happening on the page_pool side of things, right?

I seems to misunderstand it. In that case, it seems to be about defensive programming like other checking.

By looking at it more closely, it seems napi_frag_unref() call page_pool_page_put_many() directly， which means devmem seems to be bypassing the napi_safe optimization.

Can napi_frag_unref() reuse napi_pp_put_page() in order to reuse the napi_safe optimization?

I think it already does. page_pool_page_put_many() is only called if !recycle or !napi_pp_put_page(). In that case page_pool_page_put_many() is just a replacement for put_page(), because this 'page' may be an iov.

Is there a reason why not calling napi_pp_put_page() for devmem too instead of calling page_pool_page_put_many()? mem provider has a 'release_page' ops, calling page_pool_page_put_many() directly here seems to be bypassing the 'release_page' ops, which means devmem is bypassing most of the main features of page pool.

I think we're still calling napi_pp_put_page() as normal:

/** @@ -3441,13 +3466,13 @@ bool napi_pp_put_page(struct page *page, bool napi_safe); static inline void napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe) { - struct page *page = skb_frag_page(frag); - #ifdef CONFIG_PAGE_POOL - if (recycle && napi_pp_put_page(page, napi_safe)) + if (recycle && napi_pp_put_page(frag->bv_page, napi_safe)) return; + page_pool_page_put_many(frag->bv_page, 1); +#else + put_page(skb_frag_page(frag)); #endif - put_page(page); }

The code change here is to replace put_page() with page_pool_page_put_many(), only, because bv_page may be a page_pool_iov, so we need to use page_pool_page_put_many() which handles page_pool_iov correcly. I did not change whether or not napi_pp_put_page() is called. It's still called if recycle==true.

As far as I can tell, the main features of page pool:

1. Allow lockless allocation and freeing in pool->alloc cache by utilizing NAPI non-concurrent context.
2. Allow concurrent allocation and freeing in pool->ring cache by utilizing ptr_ring.
3. Allow dma map/unmap and cache sync optimization.
4. Allow detailed stats logging and tracing.
5. Allow some bulk allocation and freeing.
6. support both skb packet and xdp frame.

I am wondering what is the main features that devmem is utilizing by intergrating into page pool?

It seems the driver can just call netdev_alloc_devmem() and napi_frag_unref() can call netdev_free_devmem() directly without intergrating into page pool and it should just works too?

Maybe we should consider creating a new thin layer, in order to demux to page pool, devmem or other mem type if my suggestion does not work out too?

I went through this discussion with Jesper on RFC v2 in this thread:

https://lore.kernel.org/netdev/CAHS8izOVJGJH5WF68OsRWFKJid1_huzzUK+hpKbLcL4p...

which culminates with that email where he seems on board with the change from a performance POV, and seems on board with hiding the memory type implementation from the drivers. That thread fully goes over the tradeoffs of integrating over the page pool or creating new ones. Integrating with the page pool abstracts most of the devmem implementation (and other memory types) from the driver. It reuses page pool features like page recycling for example.

On Fri, Nov 10, 2023 at 3:20 PM Jakub Kicinski kuba@kernel.org wrote:

On Sun, 5 Nov 2023 18:44:03 -0800 Mina Almasry wrote:

...

--- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -52,6 +52,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; @@ -808,6 +810,84 @@ bool rps_may_expire_flow(struct net_device *dev, u16 rxq_index, u32 flow_id, #endif #endif /* CONFIG_RPS */

+struct netdev_dmabuf_binding {

Similar nitpick to the skbuff.h comment. Take this somewhere else, please, it doesn't need to be included in netdevice.h

...

• struct netdev_dmabuf_binding *rbinding;
  

the 'r' in rbinding stands for rx? 🤔️

reverse binding. As in usually it's netdev->binding, but the reverse map holds the bindings themselves so we can unbind them from the netdev.

On 11/5/23 7:44 PM, Mina Almasry wrote:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index eeeda849115c..1c351c138a5b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -843,6 +843,9 @@ struct netdev_dmabuf_binding { }; #ifdef CONFIG_DMA_SHARED_BUFFER +struct page_pool_iov * +netdev_alloc_devmem(struct netdev_dmabuf_binding *binding); +void netdev_free_devmem(struct page_pool_iov *ppiov);

netdev_{alloc,free}_dmabuf?

I say that because a dmabuf can be host memory, at least I am not aware of a restriction that a dmabuf is device memory.

On 11/7/23 3:10 PM, Mina Almasry wrote:

On Mon, Nov 6, 2023 at 3:44 PM David Ahern dsahern@kernel.org wrote:

...

On 11/5/23 7:44 PM, Mina Almasry wrote:

...

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index eeeda849115c..1c351c138a5b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -843,6 +843,9 @@ struct netdev_dmabuf_binding { };

#ifdef CONFIG_DMA_SHARED_BUFFER +struct page_pool_iov * +netdev_alloc_devmem(struct netdev_dmabuf_binding *binding); +void netdev_free_devmem(struct page_pool_iov *ppiov);

netdev_{alloc,free}_dmabuf?

Can do.

...

I say that because a dmabuf can be host memory, at least I am not aware of a restriction that a dmabuf is device memory.

In my limited experience dma-buf is generally device memory, and that's really its use case. CONFIG_UDMABUF is a driver that mocks dma-buf with a memfd which I think is used for testing. But I can do the rename, it's more clear anyway, I think.

config UDMABUF bool "userspace dmabuf misc driver" default n depends on DMA_SHARED_BUFFER depends on MEMFD_CREATE || COMPILE_TEST help A driver to let userspace turn memfd regions into dma-bufs. Qemu can use this to create host dmabufs for guest framebuffers.

Qemu is just a userspace process; it is no way a special one.

Treating host memory as a dmabuf should radically simplify the io_uring extension of this set. That the io_uring set needs to dive into page_pools is just wrong - complicating the design and code and pushing io_uring into a realm it does not need to be involved in.

Most (all?) of this patch set can work with any memory; only device memory is unreadable.

On 2023-11-07 15:03, Mina Almasry wrote:

On Tue, Nov 7, 2023 at 2:55 PM David Ahern dsahern@kernel.org wrote:

...

On 11/7/23 3:10 PM, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 3:44 PM David Ahern dsahern@kernel.org wrote:

...

On 11/5/23 7:44 PM, Mina Almasry wrote:

...

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index eeeda849115c..1c351c138a5b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -843,6 +843,9 @@ struct netdev_dmabuf_binding { };

#ifdef CONFIG_DMA_SHARED_BUFFER +struct page_pool_iov * +netdev_alloc_devmem(struct netdev_dmabuf_binding *binding); +void netdev_free_devmem(struct page_pool_iov *ppiov);

netdev_{alloc,free}_dmabuf?

Can do.

...

I say that because a dmabuf can be host memory, at least I am not aware of a restriction that a dmabuf is device memory.

In my limited experience dma-buf is generally device memory, and that's really its use case. CONFIG_UDMABUF is a driver that mocks dma-buf with a memfd which I think is used for testing. But I can do the rename, it's more clear anyway, I think.

config UDMABUF bool "userspace dmabuf misc driver" default n depends on DMA_SHARED_BUFFER depends on MEMFD_CREATE || COMPILE_TEST help A driver to let userspace turn memfd regions into dma-bufs. Qemu can use this to create host dmabufs for guest framebuffers.

Qemu is just a userspace process; it is no way a special one.

Treating host memory as a dmabuf should radically simplify the io_uring extension of this set.

I agree actually, and I was about to make that comment to David Wei's series once I have the time.

David, your io_uring RX zerocopy proposal actually works with devmem TCP, if you're inclined to do that instead, what you'd do roughly is (I think):

• Allocate a memfd,
• Use CONFIG_UDMABUF to create a dma-buf out of that memfd.
• Bind the dma-buf to the NIC using the netlink API in this RFC.
• Your io_uring extensions and io_uring uapi should work as-is almost

on top of this series, I think.

If you do this the incoming packets should land into your memfd, which may or may not work for you. In the future if you feel inclined to use device memory, this approach that I'm describing here would be more extensible to device memory, because you'd already be using dma-bufs for your user memory; you'd just replace one kind of dma-buf (UDMABUF) with another.

How would TCP devmem change if we no longer assume that dmabuf is device memory? Pavel will know more on the perf side, but I wouldn't want to put any if/else on the hot path if we can avoid it. I could be wrong, but right now in my mind using different memory providers solves this neatly and the driver/networking stack doesn't need to care.

Mina, I believe you said at NetDev conf that you already had an udmabuf implementation for testing. I would like to see this (you can send privately) to see how TCP devmem would handle both user memory and device memory.

...

That the io_uring set needs to dive into page_pools is just wrong - complicating the design and code and pushing io_uring into a realm it does not need to be involved in.

Most (all?) of this patch set can work with any memory; only device memory is unreadable.

On 11/10/23 7:26 AM, Pavel Begunkov wrote:

On 11/7/23 23:03, Mina Almasry wrote:

...

On Tue, Nov 7, 2023 at 2:55 PM David Ahern dsahern@kernel.org wrote:

...

On 11/7/23 3:10 PM, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 3:44 PM David Ahern dsahern@kernel.org wrote:

...

On 11/5/23 7:44 PM, Mina Almasry wrote:

...

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index eeeda849115c..1c351c138a5b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -843,6 +843,9 @@ struct netdev_dmabuf_binding {   };

#ifdef CONFIG_DMA_SHARED_BUFFER +struct page_pool_iov * +netdev_alloc_devmem(struct netdev_dmabuf_binding *binding); +void netdev_free_devmem(struct page_pool_iov *ppiov);

netdev_{alloc,free}_dmabuf?

Can do.

...

I say that because a dmabuf can be host memory, at least I am not aware of a restriction that a dmabuf is device memory.

In my limited experience dma-buf is generally device memory, and that's really its use case. CONFIG_UDMABUF is a driver that mocks dma-buf with a memfd which I think is used for testing. But I can do the rename, it's more clear anyway, I think.

config UDMABUF          bool "userspace dmabuf misc driver"          default n          depends on DMA_SHARED_BUFFER          depends on MEMFD_CREATE || COMPILE_TEST          help            A driver to let userspace turn memfd regions into dma-bufs.            Qemu can use this to create host dmabufs for guest framebuffers.

Qemu is just a userspace process; it is no way a special one.

Treating host memory as a dmabuf should radically simplify the io_uring extension of this set.

I agree actually, and I was about to make that comment to David Wei's series once I have the time.

David, your io_uring RX zerocopy proposal actually works with devmem TCP, if you're inclined to do that instead, what you'd do roughly is (I think):

That would be a Frankenstein's monster api with no good reason for it.

It brings a consistent API from a networking perspective.

io_uring should not need to be in the page pool and memory management business. Have you or David coded up the re-use of the socket APIs with dmabuf to see how much smaller it makes the io_uring change - or even walked through from a theoretical perspective?

On 2023-11-07 14:55, David Ahern wrote:

On 11/7/23 3:10 PM, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 3:44 PM David Ahern dsahern@kernel.org wrote:

...

On 11/5/23 7:44 PM, Mina Almasry wrote:

...

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index eeeda849115c..1c351c138a5b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -843,6 +843,9 @@ struct netdev_dmabuf_binding { };

#ifdef CONFIG_DMA_SHARED_BUFFER +struct page_pool_iov * +netdev_alloc_devmem(struct netdev_dmabuf_binding *binding); +void netdev_free_devmem(struct page_pool_iov *ppiov);

netdev_{alloc,free}_dmabuf?

Can do.

...

I say that because a dmabuf can be host memory, at least I am not aware of a restriction that a dmabuf is device memory.

In my limited experience dma-buf is generally device memory, and that's really its use case. CONFIG_UDMABUF is a driver that mocks dma-buf with a memfd which I think is used for testing. But I can do the rename, it's more clear anyway, I think.

config UDMABUF bool "userspace dmabuf misc driver" default n depends on DMA_SHARED_BUFFER depends on MEMFD_CREATE || COMPILE_TEST help A driver to let userspace turn memfd regions into dma-bufs. Qemu can use this to create host dmabufs for guest framebuffers.

Qemu is just a userspace process; it is no way a special one.

Treating host memory as a dmabuf should radically simplify the io_uring extension of this set. That the io_uring set needs to dive into page_pools is just wrong - complicating the design and code and pushing io_uring into a realm it does not need to be involved in.

I think our io_uring proposal will already be vastly simplified once we rebase onto Kuba's page pool memory provider API. Using udmabuf means depending on a driver designed for testing, vs io_uring's registered buffers API that's been tried and tested.

I don't have an intuitive understanding of the trade offs yet, and would need to try out udmabuf and compare vs say using our own page pool memory provider.

Most (all?) of this patch set can work with any memory; only device memory is unreadable.

...

On Mon, Nov 6, 2023 at 11:45 PM Yunsheng Lin linyunsheng@huawei.com wrote:

...

On 2023/11/6 10:44, Mina Almasry wrote:

...

+void netdev_free_devmem(struct page_pool_iov *ppiov) +{

• struct netdev_dmabuf_binding *binding = page_pool_iov_binding(ppiov);
  

• refcount_set(&ppiov->refcount, 1);
  

• if (gen_pool_has_addr(binding->chunk_pool,
  

•                       page_pool_iov_dma_addr(ppiov), PAGE_SIZE))
  

When gen_pool_has_addr() returns false, does it mean something has gone really wrong here?

Yes, good eye. gen_pool_has_addr() should never return false, but then again, gen_pool_free() BUG_ON()s if it doesn't find the address, which is an extremely severe reaction to what can be a minor bug in the accounting. I prefer to leak rather than crash the machine. It's a bit of defensive programming that is normally frowned upon, but I feel like in this case it's maybe warranted due to the very severe reaction (BUG_ON).

I would argue that why is the above defensive programming not done in the gen_pool core:)

I think gen_pool is not really not that new, and suggesting removing the BUG_ONs must have been proposed before and rejected. I'll try to do some research and maybe suggest downgrading the BUG_ON to WARN_ON, but my guess is there is some reason the maintainer wants it to be a BUG_ON.

On Wed, Nov 8, 2023 at 5:00 PM David Wei dw@davidwei.uk wrote:

On 2023-11-07 14:55, David Ahern wrote:

...

On 11/7/23 3:10 PM, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 3:44 PM David Ahern dsahern@kernel.org wrote:

...

On 11/5/23 7:44 PM, Mina Almasry wrote:

...

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index eeeda849115c..1c351c138a5b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -843,6 +843,9 @@ struct netdev_dmabuf_binding { };

#ifdef CONFIG_DMA_SHARED_BUFFER +struct page_pool_iov * +netdev_alloc_devmem(struct netdev_dmabuf_binding *binding); +void netdev_free_devmem(struct page_pool_iov *ppiov);

netdev_{alloc,free}_dmabuf?

Can do.

...

I say that because a dmabuf can be host memory, at least I am not aware of a restriction that a dmabuf is device memory.

In my limited experience dma-buf is generally device memory, and that's really its use case. CONFIG_UDMABUF is a driver that mocks dma-buf with a memfd which I think is used for testing. But I can do the rename, it's more clear anyway, I think.

config UDMABUF bool "userspace dmabuf misc driver" default n depends on DMA_SHARED_BUFFER depends on MEMFD_CREATE || COMPILE_TEST help A driver to let userspace turn memfd regions into dma-bufs. Qemu can use this to create host dmabufs for guest framebuffers.

Qemu is just a userspace process; it is no way a special one.

Treating host memory as a dmabuf should radically simplify the io_uring extension of this set. That the io_uring set needs to dive into page_pools is just wrong - complicating the design and code and pushing io_uring into a realm it does not need to be involved in.

I think our io_uring proposal will already be vastly simplified once we rebase onto Kuba's page pool memory provider API. Using udmabuf means depending on a driver designed for testing, vs io_uring's registered buffers API that's been tried and tested.

FWIW I also get an impression that udmabuf is mostly targeting testing, but I'm not aware of any deficiency that makes it concretely unsuitable for you. You be the judge.

The only quirk of udmabuf I'm aware of is that it seems to cap the max dma-buf size to 16000 pages. Not sure if that's due to a genuine technical limitation or just convenience.

I don't have an intuitive understanding of the trade offs yet, and would need to try out udmabuf and compare vs say using our own page pool memory provider.

On Wed, Nov 8, 2023 at 5:15 PM David Wei dw@davidwei.uk wrote:

How would TCP devmem change if we no longer assume that dmabuf is device memory?

It wouldn't. The code already never assumes that dmabuf is device memory. Any dma-buf should work, as far as I can tell. I'm also quite confident udmabuf works, I use it for testing.

(Jason Gunthrope is much more of an expert and may chime in to say 'some dma-buf will not work'. My primitive understanding is that we're using dma-bufs without any quirks and any dma-buf should work. I of course haven't tested all dma-bufs :D)

Pavel will know more on the perf side, but I wouldn't want to put any if/else on the hot path if we can avoid it. I could be wrong, but right now in my mind using different memory providers solves this neatly and the driver/networking stack doesn't need to care.

Mina, I believe you said at NetDev conf that you already had an udmabuf implementation for testing. I would like to see this (you can send privately) to see how TCP devmem would handle both user memory and device memory.

There is nothing to send privately. The patch series you're looking at supports udma-buf as-is, and the kselftest included with the series demonstrates devmem TCP working with udmabuf.

The only thing missing from this series is the driver support. You can see the GVE driver support for devmem TCP here:

https://github.com/torvalds/linux/compare/master...mina:linux:tcpdevmem-v3

You may need to implement devmem TCP for your driver before you can reproduce udmabuf working for yourself, though.

On 11/05, Mina Almasry wrote:

Implement a memory provider that allocates dmabuf devmem page_pool_iovs.

Support of PP_FLAG_DMA_MAP and PP_FLAG_DMA_SYNC_DEV is omitted for simplicity.

The provider receives a reference to the struct netdev_dmabuf_binding via the pool->mp_priv pointer. The driver needs to set this pointer for the provider in the page_pool_params.

The provider obtains a reference on the netdev_dmabuf_binding which guarantees the binding and the underlying mapping remains alive until the provider is destroyed.

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/net/page_pool/helpers.h | 40 +++++++++++++++++ include/net/page_pool/types.h | 10 +++++ net/core/page_pool.c | 76 +++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+)

diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h index 78cbb040af94..b93243c2a640 100644 --- a/include/net/page_pool/helpers.h +++ b/include/net/page_pool/helpers.h @@ -53,6 +53,7 @@ #define _NET_PAGE_POOL_HELPERS_H #include <net/page_pool/types.h> +#include <net/net_debug.h> #ifdef CONFIG_PAGE_POOL_STATS int page_pool_ethtool_stats_get_count(void); @@ -111,6 +112,45 @@ page_pool_iov_binding(const struct page_pool_iov *ppiov) return page_pool_iov_owner(ppiov)->binding; } +static inline int page_pool_iov_refcount(const struct page_pool_iov *ppiov) +{

• return refcount_read(&ppiov->refcount);

+}

+static inline void page_pool_iov_get_many(struct page_pool_iov *ppiov,

• 			  unsigned int count)
  

+{

• refcount_add(count, &ppiov->refcount);

+}

+void __page_pool_iov_free(struct page_pool_iov *ppiov);

+static inline void page_pool_iov_put_many(struct page_pool_iov *ppiov,

• 			  unsigned int count)
  

+{

• if (!refcount_sub_and_test(count, &ppiov->refcount))

• return;
  

• __page_pool_iov_free(ppiov);

+}

+/* page pool mm helpers */

+static inline bool page_is_page_pool_iov(const struct page *page) +{

• return (unsigned long)page & PP_DEVMEM;

+}

Speaking of bpf: one thing that might be problematic with this PP_DEVMEM bit is that it will make debugging with bpftrace a bit (more) complicated. If somebody were trying to get to that page_pool_iov from the frags, they will have to do the equivalent of page_is_page_pool_iov, but probably not a big deal? (thinking out loud)

On Mon, Nov 6, 2023 at 1:02 PM Stanislav Fomichev sdf@google.com wrote:

On 11/05, Mina Almasry wrote:

...

+static inline bool page_is_page_pool_iov(const struct page *page) +{

• return (unsigned long)page & PP_DEVMEM;
  

+}

Speaking of bpf: one thing that might be problematic with this PP_DEVMEM bit is that it will make debugging with bpftrace a bit (more) complicated. If somebody were trying to get to that page_pool_iov from the frags, they will have to do the equivalent of page_is_page_pool_iov, but probably not a big deal? (thinking out loud)

Good point, but that doesn't only apply to bpf I think. I'm guessing even debugger drgn access to the bv_page in the frag will have trouble if it's actually accessing an iov with LSB set.

But this is not specific to this use for LSB pointer trick. I think all code that currently uses LSB pointer trick will have similar troubles. In this context my humble vote is that we get such big upside from reducing code churn that it's reasonable to tolerate such side effects.

I could alleviate some of the issues by teaching drgn to do the right thing for devmem/iovs... time permitting.

On Mon, Nov 6, 2023 at 3:49 PM David Ahern dsahern@kernel.org wrote:

On 11/5/23 7:44 PM, Mina Almasry wrote:

...

diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h index 78cbb040af94..b93243c2a640 100644 --- a/include/net/page_pool/helpers.h +++ b/include/net/page_pool/helpers.h @@ -111,6 +112,45 @@ page_pool_iov_binding(const struct page_pool_iov *ppiov) return page_pool_iov_owner(ppiov)->binding; }

+static inline int page_pool_iov_refcount(const struct page_pool_iov *ppiov) +{

• return refcount_read(&ppiov->refcount);
  

+}

+static inline void page_pool_iov_get_many(struct page_pool_iov *ppiov,

•                                   unsigned int count)
  

+{

• refcount_add(count, &ppiov->refcount);
  

+}

+void __page_pool_iov_free(struct page_pool_iov *ppiov);

+static inline void page_pool_iov_put_many(struct page_pool_iov *ppiov,

•                                   unsigned int count)
  

+{

• if (!refcount_sub_and_test(count, &ppiov->refcount))
  

•         return;
  

• __page_pool_iov_free(ppiov);
  

+}

+/* page pool mm helpers */

+static inline bool page_is_page_pool_iov(const struct page *page) +{

• return (unsigned long)page & PP_DEVMEM;
  

This is another one where the code can be more generic to not force a lot changes later. e.g., PP_CUSTOM or PP_NO_PAGE. Then io_uring use case with host memory can leverage the iov pool in a similar manner.

That does mean skb->devmem needs to be a flag on the page pool and not just assume iov == device memory.

On Tue, Nov 7, 2023 at 12:00 AM Yunsheng Lin linyunsheng@huawei.com wrote:

On 2023/11/6 10:44, Mina Almasry wrote:

...

Overload the LSB of struct page* to indicate that it's a page_pool_iov.

Refactor mm calls on struct page* into helpers, and add page_pool_iov handling on those helpers. Modify callers of these mm APIs with calls to these helpers instead.

In areas where struct page* is dereferenced, add a check for special handling of page_pool_iov.

Signed-off-by: Mina Almasry almasrymina@google.com

---

include/net/page_pool/helpers.h | 74 ++++++++++++++++++++++++++++++++- net/core/page_pool.c | 63 ++++++++++++++++++++-------- 2 files changed, 118 insertions(+), 19 deletions(-)

diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h index b93243c2a640..08f1a2cc70d2 100644 --- a/include/net/page_pool/helpers.h +++ b/include/net/page_pool/helpers.h @@ -151,6 +151,64 @@ static inline struct page_pool_iov *page_to_page_pool_iov(struct page *page) return NULL; }

+static inline int page_pool_page_ref_count(struct page *page) +{

• if (page_is_page_pool_iov(page))
  

•         return page_pool_iov_refcount(page_to_page_pool_iov(page));
  

We have added a lot of 'if' for the devmem case, it would be better to make it more generic so that we can have more unified metadata handling for normal page and devmem. If we add another memory type here, do we need another 'if' here?

Maybe, not sure. I'm guessing new memory types will either be pages or iovs, so maybe no new if statements needed.

That is part of the reason I suggested using a more unified metadata for all the types of memory chunks used by page_pool.

I think your suggestion was to use struct pages for devmem. That was thoroughly considered and intensely argued about in the initial conversations regarding devmem and the initial RFC, and from the conclusions there it's extremely clear to me that devmem struct pages are categorically a no-go.

-- Thanks, Mina

On Wed, Nov 8, 2023 at 2:56 AM Yunsheng Lin linyunsheng@huawei.com wrote:

On 2023/11/8 5:56, Mina Almasry wrote:

...

On Tue, Nov 7, 2023 at 12:00 AM Yunsheng Lin linyunsheng@huawei.com wrote:

...

On 2023/11/6 10:44, Mina Almasry wrote:

...

Overload the LSB of struct page* to indicate that it's a page_pool_iov.

Refactor mm calls on struct page* into helpers, and add page_pool_iov handling on those helpers. Modify callers of these mm APIs with calls to these helpers instead.

In areas where struct page* is dereferenced, add a check for special handling of page_pool_iov.

Signed-off-by: Mina Almasry almasrymina@google.com

---

include/net/page_pool/helpers.h | 74 ++++++++++++++++++++++++++++++++- net/core/page_pool.c | 63 ++++++++++++++++++++-------- 2 files changed, 118 insertions(+), 19 deletions(-)

diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h index b93243c2a640..08f1a2cc70d2 100644 --- a/include/net/page_pool/helpers.h +++ b/include/net/page_pool/helpers.h @@ -151,6 +151,64 @@ static inline struct page_pool_iov *page_to_page_pool_iov(struct page *page) return NULL; }

+static inline int page_pool_page_ref_count(struct page *page) +{

• if (page_is_page_pool_iov(page))
  

•         return page_pool_iov_refcount(page_to_page_pool_iov(page));
  

We have added a lot of 'if' for the devmem case, it would be better to make it more generic so that we can have more unified metadata handling for normal page and devmem. If we add another memory type here, do we need another 'if' here?

Maybe, not sure. I'm guessing new memory types will either be pages or iovs, so maybe no new if statements needed.

...

That is part of the reason I suggested using a more unified metadata for all the types of memory chunks used by page_pool.

I think your suggestion was to use struct pages for devmem. That was thoroughly considered and intensely argued about in the initial conversations regarding devmem and the initial RFC, and from the conclusions there it's extremely clear to me that devmem struct pages are categorically a no-go.

Not exactly, I was wondering if adding a more abstract structure specificly for page pool makes any sense, and each mem type can add its own specific fields, net stack only see and handle the common fields so that it does not care about specific mem type, and each provider only see the and handle the specific fields belonging to it most of the time.

Ideally something like beleow:

struct netmem { /* common fields */ refcount_t refcount; struct page_pool *pp; ......

    union {
            struct devmem{
                    struct dmabuf_genpool_chunk_owner *owner;
            };

            struct other_mem{
                    ...
                    ...
            };
    };

};

But untill we completely decouple the 'struct page' from the net stack, the above seems undoable in the near term.

Agreed everything above is undoable.

But we might be able to do something as folio is doing now, mm subsystem is still seeing 'struct folio/page', but other subsystem like slab is using 'struct slab', and there is still some common fields shared between 'struct folio' and 'struct slab'.

In my eyes this is almost exactly what I suggested in RFC v1 and got immediately nacked with no room to negotiate. What we did for v1 is to allocate struct pages for dma-buf to make dma-bufs look like struct page to mm subsystem. Almost exactly what you're describing above. It's a no-go. I don't think renaming struct page to netmem is going to move the needle (it also re-introduces code-churn). What I feel like I learnt is that dma-bufs are not struct pages and can't be made to look like one, I think.

As the netmem patchset, is devmem able to reuse the below 'struct netmem' and rename it to 'struct page_pool_iov'?

I don't think so. For the reasons above, but also practically it immediately falls apart. Consider this field in netmem:

+ * @flags: The same as the page flags. Do not use directly.

dma-buf don't have or support page-flags, and making dma-buf looks like they support page flags or any page-like features (other than dma_addr) seems extremely unacceptable to mm folks.

So that 'struct page' for normal memory and 'struct page_pool_iov' for devmem share the common fields used by page pool and net stack?

Are you suggesting that we'd cast a netmem* to a page* and call core mm APIs on it? It's basically what was happening with RFC v1, where things that are not struct pages were made to look like struct pages.

Also, there isn't much upside for what you're suggesting, I think. For example I can align the refcount variable in struct page_pool_iov with the refcount in struct page so that this works:

put_page((struct page*)ppiov);

but it's a disaster. Because put_page() will call __put_page() if the page is freed, and __put_page() will try to return the page to the buddy allocator!

And we might be able to reuse the 'flags', '_pp_mapping_pad' and '_mapcount' for specific mem provider, which is enough for the devmem only requiring a single pointer to point to it's owner?

All the above seems quite similar to RFC v1 again, using netmem instead of struct page. In RFC v1 we re-used zone_device_data() for the dma-buf owner equivalent.

https://lkml.kernel.org/netdev/20230105214631.3939268-2-willy@infradead.org/

+/**

• • struct netmem - A memory allocation from a &struct page_pool.
• • @flags: The same as the page flags. Do not use directly.
• • @pp_magic: Magic value to avoid recycling non page_pool allocated pages.
• • @pp: The page pool this netmem was allocated from.
• • @dma_addr: Call netmem_get_dma_addr() to read this value.
• • @dma_addr_upper: Might need to be 64-bit on 32-bit architectures.
• • @pp_frag_count: For frag page support, not supported in 32-bit
• • architectures with 64-bit DMA.
• • @_mapcount: Do not access this member directly.
• • @_refcount: Do not access this member directly. Read it using
• • netmem_ref_count() and manipulate it with netmem_get() and netmem_put().
• • This struct overlays struct page for now. Do not modify without a
• • good understanding of the issues.
• */

+struct netmem {

•   unsigned long flags;
  

•   unsigned long pp_magic;
  

•   struct page_pool *pp;
  

•   /* private: no need to document this padding */
  

•   unsigned long _pp_mapping_pad;  /* aliases with folio->mapping */
  

•   /* public: */
  

•   unsigned long dma_addr;
  

•   union {
  

•           unsigned long dma_addr_upper;
  

•           atomic_long_t pp_frag_count;
  

•   };
  

•   atomic_t _mapcount;
  

•   atomic_t _refcount;
  

+};

If we do that, it seems we might be able to allow net stack and page pool to see the metadata for devmem chunk as 'struct page', and may be able to aovid most of the 'if' checking in net stack and page pool?

...

-- Thanks, Mina

.

On Sun, 2023-11-05 at 18:44 -0800, Mina Almasry wrote:

Overload the LSB of struct page* to indicate that it's a page_pool_iov.

Refactor mm calls on struct page* into helpers, and add page_pool_iov handling on those helpers. Modify callers of these mm APIs with calls to these helpers instead.

In areas where struct page* is dereferenced, add a check for special handling of page_pool_iov.

Signed-off-by: Mina Almasry almasrymina@google.com

---

include/net/page_pool/helpers.h | 74 ++++++++++++++++++++++++++++++++- net/core/page_pool.c | 63 ++++++++++++++++++++-------- 2 files changed, 118 insertions(+), 19 deletions(-)

diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h index b93243c2a640..08f1a2cc70d2 100644 --- a/include/net/page_pool/helpers.h +++ b/include/net/page_pool/helpers.h @@ -151,6 +151,64 @@ static inline struct page_pool_iov *page_to_page_pool_iov(struct page *page) return NULL; } +static inline int page_pool_page_ref_count(struct page *page) +{

• if (page_is_page_pool_iov(page))

• return page_pool_iov_refcount(page_to_page_pool_iov(page));
  

• return page_ref_count(page);

+}

+static inline void page_pool_page_get_many(struct page *page,

• 			   unsigned int count)
  

+{

• if (page_is_page_pool_iov(page))

• return page_pool_iov_get_many(page_to_page_pool_iov(page),
  

• 			      count);
  

• return page_ref_add(page, count);

+}

+static inline void page_pool_page_put_many(struct page *page,

• 			   unsigned int count)
  

+{

• if (page_is_page_pool_iov(page))

• return page_pool_iov_put_many(page_to_page_pool_iov(page),
  

• 			      count);
  

• if (count > 1)

• page_ref_sub(page, count - 1);
  

• put_page(page);

+}

+static inline bool page_pool_page_is_pfmemalloc(struct page *page) +{

• if (page_is_page_pool_iov(page))

• return false;
  

• return page_is_pfmemalloc(page);

+}

+static inline bool page_pool_page_is_pref_nid(struct page *page, int pref_nid) +{

• /* Assume page_pool_iov are on the preferred node without actually

• * checking...
  

• *
  

• * This check is only used to check for recycling memory in the page
  

• * pool's fast paths. Currently the only implementation of page_pool_iov
  

• * is dmabuf device memory. It's a deliberate decision by the user to
  

• * bind a certain dmabuf to a certain netdev, and the netdev rx queue
  

• * would not be able to reallocate memory from another dmabuf that
  

• * exists on the preferred node, so, this check doesn't make much sense
  

• * in this case. Assume all page_pool_iovs can be recycled for now.
  

• */
  

• if (page_is_page_pool_iov(page))

• return true;
  

• return page_to_nid(page) == pref_nid;

+}

/**

• page_pool_dev_alloc_pages() - allocate a page.
• @pool: pool from which to allocate

@@ -301,6 +359,9 @@ static inline long page_pool_defrag_page(struct page *page, long nr) { long ret;

• if (page_is_page_pool_iov(page))

• return -EINVAL;
  

• /* If nr == pp_frag_count then we have cleared all remaining
  • references to the page:
  • 1. 'n == 1': no need to actually overwrite it.

@@ -431,7 +492,12 @@ static inline void page_pool_free_va(struct page_pool *pool, void *va, */ static inline dma_addr_t page_pool_get_dma_addr(struct page *page) {

• dma_addr_t ret = page->dma_addr;

• dma_addr_t ret;
• if (page_is_page_pool_iov(page))

• return page_pool_iov_dma_addr(page_to_page_pool_iov(page));
  

Should the above conditional be guarded by the page_pool_mem_providers static key? this looks like fast-path. Same question for the refcount helper above.

Minor nit: possibly cache 'page_is_page_pool_iov(page)' to make the code more readable.

• ret = page->dma_addr;

if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) ret <<= PAGE_SHIFT; @@ -441,6 +507,12 @@ static inline dma_addr_t page_pool_get_dma_addr(struct page *page) static inline bool page_pool_set_dma_addr(struct page *page, dma_addr_t addr) {

• /* page_pool_iovs are mapped and their dma-addr can't be modified. */
• if (page_is_page_pool_iov(page)) {

• DEBUG_NET_WARN_ON_ONCE(true);
  

• return false;
  

• }

Quickly skimming over the page_pool_code it looks like page_pool_set_dma_addr() usage is guarded by the PP_FLAG_DMA_MAP page pool flag. Could the device mem provider enforce such flag being cleared on the page pool?

• if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) { page->dma_addr = addr >> PAGE_SHIFT;

diff --git a/net/core/page_pool.c b/net/core/page_pool.c index 138ddea0b28f..d211996d423b 100644 --- a/net/core/page_pool.cnn +++ b/net/core/page_pool.c @@ -317,7 +317,7 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool) if (unlikely(!page)) break;

• if (likely(page_to_nid(page) == pref_nid)) {
  

• if (likely(page_pool_page_is_pref_nid(page, pref_nid))) {
  pool->alloc.cache[pool->alloc.count++] = page;
  

  } else { /* NUMA mismatch;

@@ -362,7 +362,15 @@ static void page_pool_dma_sync_for_device(struct page_pool *pool, struct page *page, unsigned int dma_sync_size) {

• dma_addr_t dma_addr = page_pool_get_dma_addr(page);

• dma_addr_t dma_addr;
• /* page_pool_iov memory provider do not support PP_FLAG_DMA_SYNC_DEV */
• if (page_is_page_pool_iov(page)) {

• DEBUG_NET_WARN_ON_ONCE(true);
  

• return;
  

• }

Similar to the above point, mutatis mutandis.

• dma_addr = page_pool_get_dma_addr(page);

dma_sync_size = min(dma_sync_size, pool->p.max_len); dma_sync_single_range_for_device(pool->p.dev, dma_addr, @@ -374,6 +382,12 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page) { dma_addr_t dma;

• if (page_is_page_pool_iov(page)) {

• /* page_pool_iovs are already mapped */
  

• DEBUG_NET_WARN_ON_ONCE(true);
  

• return true;
  

• }

Ditto.

Cheers,

Paolo

On Thu, Nov 9, 2023 at 1:15 AM Paolo Abeni pabeni@redhat.com wrote:

On Sun, 2023-11-05 at 18:44 -0800, Mina Almasry wrote: [...]

...

@@ -3421,7 +3446,7 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag) */ static inline void __skb_frag_ref(skb_frag_t *frag) {

• get_page(skb_frag_page(frag));
  

• page_pool_page_get_many(frag->bv_page, 1);
  

I guess the above needs #ifdef CONFIG_PAGE_POOL guards and explicit skb_frag_is_page_pool_iov() check ?

It doesn't actually. page_pool_page_* helpers are compiled in regardless of CONFIG_PAGE_POOL, and handle both page_pool_iov* & page* just fine (the checking happens inside the function).

You may yell at me that it's too confusing... I somewhat agree, but I'm unsure of what is a better name or location for the helpers. The helpers handle (page_pool_iov* || page*) gracefully, so they seem to belong in the page pool for me, but it is indeed surprising/confusing that these helpers are available even if !CONFIG_PAGE_POOL.

Cheers,

Paolo

On Fri, Nov 10, 2023 at 3:19 PM Jakub Kicinski kuba@kernel.org wrote:

On Sun, 5 Nov 2023 18:44:07 -0800 Mina Almasry wrote:

...

#include <net/net_debug.h> #include <net/dropreason-core.h> +#include <net/page_pool/types.h> +#include <net/page_pool/helpers.h>

...

/**

• DOC: skb checksums

@@ -3402,15 +3404,38 @@ static inline void skb_frag_off_copy(skb_frag_t *fragto, fragto->bv_offset = fragfrom->bv_offset; }

+/* Returns true if the skb_frag contains a page_pool_iov. */ +static inline bool skb_frag_is_page_pool_iov(const skb_frag_t *frag) +{

• return page_is_page_pool_iov(frag->bv_page);
  

+}

Maybe we can create a new header? For skb + page pool.

skbuff.h is included by 1/4th of the kernel objects, we should not be adding dependencies to this header, it really slows down incremental builds.

My issue here is that all these skb helpers call each other so I end up having to move a lot of the unrelated skb helpers to this new header (maybe that is acceptable but it feels weird).

What I could do here is move all the page_pool_page|iov_* helpers to a minimal header, and include only that one from skbuff.h, rather than including all of net/page_pool/helpers.h

On 11/6/23 11:47 AM, Stanislav Fomichev wrote:

On 11/05, Mina Almasry wrote:

...

For device memory TCP, we expect the skb headers to be available in host memory for access, and we expect the skb frags to be in device memory and unaccessible to the host. We expect there to be no mixing and matching of device memory frags (unaccessible) with host memory frags (accessible) in the same skb.

Add a skb->devmem flag which indicates whether the frags in this skb are device memory frags or not.

__skb_fill_page_desc() now checks frags added to skbs for page_pool_iovs, and marks the skb as skb->devmem accordingly.

Add checks through the network stack to avoid accessing the frags of devmem skbs and avoid coalescing devmem skbs with non devmem skbs.

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/skbuff.h | 14 +++++++- include/net/tcp.h | 5 +-- net/core/datagram.c | 6 ++++ net/core/gro.c | 5 ++- net/core/skbuff.c | 77 ++++++++++++++++++++++++++++++++++++------ net/ipv4/tcp.c | 6 ++++ net/ipv4/tcp_input.c | 13 +++++-- net/ipv4/tcp_output.c | 5 ++- net/packet/af_packet.c | 4 +-- 9 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 1fae276c1353..8fb468ff8115 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -805,6 +805,8 @@ typedef unsigned char *sk_buff_data_t;

• @csum_level: indicates the number of consecutive checksums found in

• the packet minus one that have been verified as
  

• CHECKSUM_UNNECESSARY (max 3)
  

• • @devmem: indicates that all the fragments in this skb are backed by

• • device memory.
    

  • @dst_pending_confirm: need to confirm neighbour
  • @decrypted: Decrypted SKB
  • @slow_gro: state present at GRO time, slower prepare step required

@@ -991,7 +993,7 @@ struct sk_buff { #if IS_ENABLED(CONFIG_IP_SCTP) __u8 csum_not_inet:1; #endif

• __u8 devmem:1;

#if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS) __u16 tc_index; /* traffic control index */ #endif @@ -1766,6 +1768,12 @@ static inline void skb_zcopy_downgrade_managed(struct sk_buff *skb) __skb_zcopy_downgrade_managed(skb); } +/* Return true if frags in this skb are not readable by the host. */ +static inline bool skb_frags_not_readable(const struct sk_buff *skb) +{

• return skb->devmem;

bikeshedding: should we also rename 'devmem' sk_buff flag to 'not_readable'? It better communicates the fact that the stack shouldn't dereference the frags (because it has 'devmem' fragments or for some other potential future reason).

+1.

Also, the flag on the skb is an optimization - a high level signal that one or more frags is in unreadable memory. There is no requirement that all of the frags are in the same memory type.

On 11/06, Mina Almasry wrote:

On Mon, Nov 6, 2023 at 11:34 AM David Ahern dsahern@kernel.org wrote:

...

On 11/6/23 11:47 AM, Stanislav Fomichev wrote:

...

On 11/05, Mina Almasry wrote:

...

For device memory TCP, we expect the skb headers to be available in host memory for access, and we expect the skb frags to be in device memory and unaccessible to the host. We expect there to be no mixing and matching of device memory frags (unaccessible) with host memory frags (accessible) in the same skb.

Add a skb->devmem flag which indicates whether the frags in this skb are device memory frags or not.

__skb_fill_page_desc() now checks frags added to skbs for page_pool_iovs, and marks the skb as skb->devmem accordingly.

Add checks through the network stack to avoid accessing the frags of devmem skbs and avoid coalescing devmem skbs with non devmem skbs.

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/skbuff.h | 14 +++++++- include/net/tcp.h | 5 +-- net/core/datagram.c | 6 ++++ net/core/gro.c | 5 ++- net/core/skbuff.c | 77 ++++++++++++++++++++++++++++++++++++------ net/ipv4/tcp.c | 6 ++++ net/ipv4/tcp_input.c | 13 +++++-- net/ipv4/tcp_output.c | 5 ++- net/packet/af_packet.c | 4 +-- 9 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 1fae276c1353..8fb468ff8115 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -805,6 +805,8 @@ typedef unsigned char *sk_buff_data_t;

• @csum_level: indicates the number of consecutive checksums found in

•      the packet minus one that have been verified as
  

•      CHECKSUM_UNNECESSARY (max 3)
  

• • @devmem: indicates that all the fragments in this skb are backed by

• •      device memory.
    

  • @dst_pending_confirm: need to confirm neighbour
  • @decrypted: Decrypted SKB
  • @slow_gro: state present at GRO time, slower prepare step required

@@ -991,7 +993,7 @@ struct sk_buff { #if IS_ENABLED(CONFIG_IP_SCTP) __u8 csum_not_inet:1; #endif

• __u8 devmem:1;

#if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS) __u16 tc_index; /* traffic control index */ #endif @@ -1766,6 +1768,12 @@ static inline void skb_zcopy_downgrade_managed(struct sk_buff *skb) __skb_zcopy_downgrade_managed(skb); }

+/* Return true if frags in this skb are not readable by the host. */ +static inline bool skb_frags_not_readable(const struct sk_buff *skb) +{

• return skb->devmem;

bikeshedding: should we also rename 'devmem' sk_buff flag to 'not_readable'? It better communicates the fact that the stack shouldn't dereference the frags (because it has 'devmem' fragments or for some other potential future reason).

+1.

Also, the flag on the skb is an optimization - a high level signal that one or more frags is in unreadable memory. There is no requirement that all of the frags are in the same memory type.

David: maybe there should be such a requirement (that they all are unreadable)? Might be easier to support initially; we can relax later on.

The flag indicates that the skb contains all devmem dma-buf memory specifically, not generic 'not_readable' frags as the comment says:

• • @devmem: indicates that all the fragments in this skb are backed by
    

• •         device memory.
    
    

The reason it's not a generic 'not_readable' flag is because handing off a generic not_readable skb to the userspace is semantically not what we're doing. recvmsg() is augmented in this patch series to return a devmem skb to the user via a cmsg_devmem struct which refers specifically to the memory in the dma-buf. recvmsg() in this patch series is not augmented to give any 'not_readable' skb to the userspace.

IMHO skb->devmem + an skb_frags_not_readable() as implemented is correct. If a new type of unreadable skbs are introduced to the stack, I imagine the stack would implement:

1. new header flag: skb->newmem

static inline bool skb_frags_not_readable(const struct skb_buff *skb) { return skb->devmem || skb->newmem; }

3. tcp_recvmsg_devmem() would handle skb->devmem skbs is in this patch

series, but tcp_recvmsg_newmem() would handle skb->newmem skbs.

You copy it to the userspace in a special way because your frags are page_is_page_pool_iov(). I agree with David, the skb bit is just and optimization.

For most of the core stack, it doesn't matter why your skb is not readable. For a few places where it matters (recvmsg?), you can double-check your frags (all or some) with page_is_page_pool_iov.

Unrelated: we probably need socket to dmabuf association as well (via netlink or something). We are fundamentally receiving into and sending from a dmabuf (devmem == dmabuf). And once you have this association, recvmsg shouldn't need any new special flags.

On 11/06, Mina Almasry wrote:

On Mon, Nov 6, 2023 at 1:59 PM Stanislav Fomichev sdf@google.com wrote:

...

On 11/06, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 11:34 AM David Ahern dsahern@kernel.org wrote:

...

On 11/6/23 11:47 AM, Stanislav Fomichev wrote:

...

On 11/05, Mina Almasry wrote:

...

For device memory TCP, we expect the skb headers to be available in host memory for access, and we expect the skb frags to be in device memory and unaccessible to the host. We expect there to be no mixing and matching of device memory frags (unaccessible) with host memory frags (accessible) in the same skb.

Add a skb->devmem flag which indicates whether the frags in this skb are device memory frags or not.

__skb_fill_page_desc() now checks frags added to skbs for page_pool_iovs, and marks the skb as skb->devmem accordingly.

Add checks through the network stack to avoid accessing the frags of devmem skbs and avoid coalescing devmem skbs with non devmem skbs.

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/skbuff.h | 14 +++++++- include/net/tcp.h | 5 +-- net/core/datagram.c | 6 ++++ net/core/gro.c | 5 ++- net/core/skbuff.c | 77 ++++++++++++++++++++++++++++++++++++------ net/ipv4/tcp.c | 6 ++++ net/ipv4/tcp_input.c | 13 +++++-- net/ipv4/tcp_output.c | 5 ++- net/packet/af_packet.c | 4 +-- 9 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 1fae276c1353..8fb468ff8115 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -805,6 +805,8 @@ typedef unsigned char *sk_buff_data_t;

• @csum_level: indicates the number of consecutive checksums found in

•      the packet minus one that have been verified as
  

•      CHECKSUM_UNNECESSARY (max 3)
  

• • @devmem: indicates that all the fragments in this skb are backed by

• •      device memory.
    

  • @dst_pending_confirm: need to confirm neighbour
  • @decrypted: Decrypted SKB
  • @slow_gro: state present at GRO time, slower prepare step required

@@ -991,7 +993,7 @@ struct sk_buff { #if IS_ENABLED(CONFIG_IP_SCTP) __u8 csum_not_inet:1; #endif

• __u8 devmem:1;

#if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS) __u16 tc_index; /* traffic control index */ #endif @@ -1766,6 +1768,12 @@ static inline void skb_zcopy_downgrade_managed(struct sk_buff *skb) __skb_zcopy_downgrade_managed(skb); }

+/* Return true if frags in this skb are not readable by the host. */ +static inline bool skb_frags_not_readable(const struct sk_buff *skb) +{

• return skb->devmem;

bikeshedding: should we also rename 'devmem' sk_buff flag to 'not_readable'? It better communicates the fact that the stack shouldn't dereference the frags (because it has 'devmem' fragments or for some other potential future reason).

+1.

Also, the flag on the skb is an optimization - a high level signal that one or more frags is in unreadable memory. There is no requirement that all of the frags are in the same memory type.

David: maybe there should be such a requirement (that they all are unreadable)? Might be easier to support initially; we can relax later on.

Currently devmem == not_readable, and the restriction is that all the frags in the same skb must be either all readable or all unreadable (all devmem or all non-devmem).

...

...

The flag indicates that the skb contains all devmem dma-buf memory specifically, not generic 'not_readable' frags as the comment says:

• • @devmem: indicates that all the fragments in this skb are backed by
    

• •         device memory.
    
    

The reason it's not a generic 'not_readable' flag is because handing off a generic not_readable skb to the userspace is semantically not what we're doing. recvmsg() is augmented in this patch series to return a devmem skb to the user via a cmsg_devmem struct which refers specifically to the memory in the dma-buf. recvmsg() in this patch series is not augmented to give any 'not_readable' skb to the userspace.

IMHO skb->devmem + an skb_frags_not_readable() as implemented is correct. If a new type of unreadable skbs are introduced to the stack, I imagine the stack would implement:

1. new header flag: skb->newmem

static inline bool skb_frags_not_readable(const struct skb_buff *skb) { return skb->devmem || skb->newmem; }

3. tcp_recvmsg_devmem() would handle skb->devmem skbs is in this patch

series, but tcp_recvmsg_newmem() would handle skb->newmem skbs.

You copy it to the userspace in a special way because your frags are page_is_page_pool_iov(). I agree with David, the skb bit is just and optimization.

For most of the core stack, it doesn't matter why your skb is not readable. For a few places where it matters (recvmsg?), you can double-check your frags (all or some) with page_is_page_pool_iov.

I see, we can do that then. I.e. make the header flag 'not_readable' and check the frags to decide to delegate to tcp_recvmsg_devmem() or something else. We can even assume not_readable == devmem because currently devmem is the only type of unreadable frag currently.

...

Unrelated: we probably need socket to dmabuf association as well (via netlink or something).

Not sure this is possible. The dma-buf is bound to the rx-queue, and any packets that land on that rx-queue are bound to that dma-buf, regardless of which socket that packet belongs to. So the association IMO must be rx-queue to dma-buf, not socket to dma-buf.

But there is still always 1 dmabuf to 1 socket association (on rx), right? Because otherwise, there is no way currently to tell, at recvmsg, which dmabuf the received token belongs to.

So why not have a separate control channel action to say: this socket fd is supposed to receive into this dmabuf fd? This action would put the socket into permanent 'MSG_SOCK_DEVMEM' mode. Maybe you can also put some checks at the lower level to to enforce this dmabuf association. (to avoid any potential issues with flow steering)

We'll still have dmabuf to rx-queue association because of various reasons..

On Mon, Nov 6, 2023 at 3:27 PM Mina Almasry almasrymina@google.com wrote:

On Mon, Nov 6, 2023 at 2:59 PM Stanislav Fomichev sdf@google.com wrote:

...

On 11/06, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 1:59 PM Stanislav Fomichev sdf@google.com wrote:

...

On 11/06, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 11:34 AM David Ahern dsahern@kernel.org wrote:

...

On 11/6/23 11:47 AM, Stanislav Fomichev wrote: > On 11/05, Mina Almasry wrote: >> For device memory TCP, we expect the skb headers to be available in host >> memory for access, and we expect the skb frags to be in device memory >> and unaccessible to the host. We expect there to be no mixing and >> matching of device memory frags (unaccessible) with host memory frags >> (accessible) in the same skb. >> >> Add a skb->devmem flag which indicates whether the frags in this skb >> are device memory frags or not. >> >> __skb_fill_page_desc() now checks frags added to skbs for page_pool_iovs, >> and marks the skb as skb->devmem accordingly. >> >> Add checks through the network stack to avoid accessing the frags of >> devmem skbs and avoid coalescing devmem skbs with non devmem skbs. >> >> 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/skbuff.h | 14 +++++++- >> include/net/tcp.h | 5 +-- >> net/core/datagram.c | 6 ++++ >> net/core/gro.c | 5 ++- >> net/core/skbuff.c | 77 ++++++++++++++++++++++++++++++++++++------ >> net/ipv4/tcp.c | 6 ++++ >> net/ipv4/tcp_input.c | 13 +++++-- >> net/ipv4/tcp_output.c | 5 ++- >> net/packet/af_packet.c | 4 +-- >> 9 files changed, 115 insertions(+), 20 deletions(-) >> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >> index 1fae276c1353..8fb468ff8115 100644 >> --- a/include/linux/skbuff.h >> +++ b/include/linux/skbuff.h >> @@ -805,6 +805,8 @@ typedef unsigned char *sk_buff_data_t; >> * @csum_level: indicates the number of consecutive checksums found in >> * the packet minus one that have been verified as >> * CHECKSUM_UNNECESSARY (max 3) >> + * @devmem: indicates that all the fragments in this skb are backed by >> + * device memory. >> * @dst_pending_confirm: need to confirm neighbour >> * @decrypted: Decrypted SKB >> * @slow_gro: state present at GRO time, slower prepare step required >> @@ -991,7 +993,7 @@ struct sk_buff { >> #if IS_ENABLED(CONFIG_IP_SCTP) >> __u8 csum_not_inet:1; >> #endif >> - >> + __u8 devmem:1; >> #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS) >> __u16 tc_index; /* traffic control index */ >> #endif >> @@ -1766,6 +1768,12 @@ static inline void skb_zcopy_downgrade_managed(struct sk_buff *skb) >> __skb_zcopy_downgrade_managed(skb); >> } >> >> +/* Return true if frags in this skb are not readable by the host. */ >> +static inline bool skb_frags_not_readable(const struct sk_buff *skb) >> +{ >> + return skb->devmem; > > bikeshedding: should we also rename 'devmem' sk_buff flag to 'not_readable'? > It better communicates the fact that the stack shouldn't dereference the > frags (because it has 'devmem' fragments or for some other potential > future reason).

+1.

Also, the flag on the skb is an optimization - a high level signal that one or more frags is in unreadable memory. There is no requirement that all of the frags are in the same memory type.

David: maybe there should be such a requirement (that they all are unreadable)? Might be easier to support initially; we can relax later on.

Currently devmem == not_readable, and the restriction is that all the frags in the same skb must be either all readable or all unreadable (all devmem or all non-devmem).

...

...

The flag indicates that the skb contains all devmem dma-buf memory specifically, not generic 'not_readable' frags as the comment says:

• • @devmem: indicates that all the fragments in this skb are backed by
    

• •         device memory.
    
    

The reason it's not a generic 'not_readable' flag is because handing off a generic not_readable skb to the userspace is semantically not what we're doing. recvmsg() is augmented in this patch series to return a devmem skb to the user via a cmsg_devmem struct which refers specifically to the memory in the dma-buf. recvmsg() in this patch series is not augmented to give any 'not_readable' skb to the userspace.

IMHO skb->devmem + an skb_frags_not_readable() as implemented is correct. If a new type of unreadable skbs are introduced to the stack, I imagine the stack would implement:

1. new header flag: skb->newmem

static inline bool skb_frags_not_readable(const struct skb_buff *skb) { return skb->devmem || skb->newmem; }

3. tcp_recvmsg_devmem() would handle skb->devmem skbs is in this patch

series, but tcp_recvmsg_newmem() would handle skb->newmem skbs.

You copy it to the userspace in a special way because your frags are page_is_page_pool_iov(). I agree with David, the skb bit is just and optimization.

For most of the core stack, it doesn't matter why your skb is not readable. For a few places where it matters (recvmsg?), you can double-check your frags (all or some) with page_is_page_pool_iov.

I see, we can do that then. I.e. make the header flag 'not_readable' and check the frags to decide to delegate to tcp_recvmsg_devmem() or something else. We can even assume not_readable == devmem because currently devmem is the only type of unreadable frag currently.

...

Unrelated: we probably need socket to dmabuf association as well (via netlink or something).

Not sure this is possible. The dma-buf is bound to the rx-queue, and any packets that land on that rx-queue are bound to that dma-buf, regardless of which socket that packet belongs to. So the association IMO must be rx-queue to dma-buf, not socket to dma-buf.

But there is still always 1 dmabuf to 1 socket association (on rx), right? Because otherwise, there is no way currently to tell, at recvmsg, which dmabuf the received token belongs to.

Yes, but this 1 dma-buf to 1 socket association happens because the user binds the dma-buf to an rx-queue and configures flow steering of the socket to that rx-queue.

It's still fixed and won't change during the socket lifetime, right? And the socket has to know this association; otherwise those tokens are useless since they don't carry anything to identify the dmabuf.

I think my other issue with MSG_SOCK_DEVMEM being on recvmsg is that it somehow implies that I have an option of passing or not passing it for an individual system call. If we know that we're going to use dmabuf with the socket, maybe we should move this flag to the socket() syscall?

fd = socket(AF_INET6, SOCK_STREAM, SOCK_DEVMEM);

?

...

So why not have a separate control channel action to say: this socket fd is supposed to receive into this dmabuf fd? This action would put the socket into permanent 'MSG_SOCK_DEVMEM' mode. Maybe you can also put some checks at the lower level to to enforce this dmabuf association. (to avoid any potential issues with flow steering)

setsockopt(SO_DEVMEM_ASSERT_DMA_BUF, dmabuf_fd)? Sounds interesting, but maybe a bit of a weird API to me. Because the API can't enforce the socket to receive packets on a dma-buf (rx-queue binding + flow steering does that), but the API can assert that incoming packets are received on said dma-buf. I guess it would check packets before they are acked and would drop packets that landed on the wrong queue.

I'm a bit unsure about defensively programming features (and uapi no less) to 'avoid any potential issues with flow steering'. Flow steering is supposed to work.

Also if we wanted to defensively program something to avoid flow steering issues, then I'd suggest adding to cmsg_devmem the dma-buf fd that the data is on, not this setsockopt() that asserts. IMO it's a weird API for the userspace to ask the kernel to assert some condition (at least I haven't seen it before or commonly).

But again, in general, I'm a bit unsure about defensively designing uapi around a feature like flow steering that's supposed to work.

On 11/06, Willem de Bruijn wrote:

On Mon, Nov 6, 2023 at 3:55 PM Stanislav Fomichev sdf@google.com wrote:

...

On Mon, Nov 6, 2023 at 3:27 PM Mina Almasry almasrymina@google.com wrote:

...

On Mon, Nov 6, 2023 at 2:59 PM Stanislav Fomichev sdf@google.com wrote:

...

On 11/06, Mina Almasry wrote:

...

On Mon, Nov 6, 2023 at 1:59 PM Stanislav Fomichev sdf@google.com wrote:

...

On 11/06, Mina Almasry wrote: > On Mon, Nov 6, 2023 at 11:34 AM David Ahern dsahern@kernel.org wrote: > > > > On 11/6/23 11:47 AM, Stanislav Fomichev wrote: > > > On 11/05, Mina Almasry wrote: > > >> For device memory TCP, we expect the skb headers to be available in host > > >> memory for access, and we expect the skb frags to be in device memory > > >> and unaccessible to the host. We expect there to be no mixing and > > >> matching of device memory frags (unaccessible) with host memory frags > > >> (accessible) in the same skb. > > >> > > >> Add a skb->devmem flag which indicates whether the frags in this skb > > >> are device memory frags or not. > > >> > > >> __skb_fill_page_desc() now checks frags added to skbs for page_pool_iovs, > > >> and marks the skb as skb->devmem accordingly. > > >> > > >> Add checks through the network stack to avoid accessing the frags of > > >> devmem skbs and avoid coalescing devmem skbs with non devmem skbs. > > >> > > >> 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/skbuff.h | 14 +++++++- > > >> include/net/tcp.h | 5 +-- > > >> net/core/datagram.c | 6 ++++ > > >> net/core/gro.c | 5 ++- > > >> net/core/skbuff.c | 77 ++++++++++++++++++++++++++++++++++++------ > > >> net/ipv4/tcp.c | 6 ++++ > > >> net/ipv4/tcp_input.c | 13 +++++-- > > >> net/ipv4/tcp_output.c | 5 ++- > > >> net/packet/af_packet.c | 4 +-- > > >> 9 files changed, 115 insertions(+), 20 deletions(-) > > >> > > >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > >> index 1fae276c1353..8fb468ff8115 100644 > > >> --- a/include/linux/skbuff.h > > >> +++ b/include/linux/skbuff.h > > >> @@ -805,6 +805,8 @@ typedef unsigned char *sk_buff_data_t; > > >> * @csum_level: indicates the number of consecutive checksums found in > > >> * the packet minus one that have been verified as > > >> * CHECKSUM_UNNECESSARY (max 3) > > >> + * @devmem: indicates that all the fragments in this skb are backed by > > >> + * device memory. > > >> * @dst_pending_confirm: need to confirm neighbour > > >> * @decrypted: Decrypted SKB > > >> * @slow_gro: state present at GRO time, slower prepare step required > > >> @@ -991,7 +993,7 @@ struct sk_buff { > > >> #if IS_ENABLED(CONFIG_IP_SCTP) > > >> __u8 csum_not_inet:1; > > >> #endif > > >> - > > >> + __u8 devmem:1; > > >> #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS) > > >> __u16 tc_index; /* traffic control index */ > > >> #endif > > >> @@ -1766,6 +1768,12 @@ static inline void skb_zcopy_downgrade_managed(struct sk_buff *skb) > > >> __skb_zcopy_downgrade_managed(skb); > > >> } > > >> > > >> +/* Return true if frags in this skb are not readable by the host. */ > > >> +static inline bool skb_frags_not_readable(const struct sk_buff *skb) > > >> +{ > > >> + return skb->devmem; > > > > > > bikeshedding: should we also rename 'devmem' sk_buff flag to 'not_readable'? > > > It better communicates the fact that the stack shouldn't dereference the > > > frags (because it has 'devmem' fragments or for some other potential > > > future reason). > > > > +1. > > > > Also, the flag on the skb is an optimization - a high level signal that > > one or more frags is in unreadable memory. There is no requirement that > > all of the frags are in the same memory type.

David: maybe there should be such a requirement (that they all are unreadable)? Might be easier to support initially; we can relax later on.

Currently devmem == not_readable, and the restriction is that all the frags in the same skb must be either all readable or all unreadable (all devmem or all non-devmem).

...

> The flag indicates that the skb contains all devmem dma-buf memory > specifically, not generic 'not_readable' frags as the comment says: > > + * @devmem: indicates that all the fragments in this skb are backed by > + * device memory. > > The reason it's not a generic 'not_readable' flag is because handing > off a generic not_readable skb to the userspace is semantically not > what we're doing. recvmsg() is augmented in this patch series to > return a devmem skb to the user via a cmsg_devmem struct which refers > specifically to the memory in the dma-buf. recvmsg() in this patch > series is not augmented to give any 'not_readable' skb to the > userspace. > > IMHO skb->devmem + an skb_frags_not_readable() as implemented is > correct. If a new type of unreadable skbs are introduced to the stack, > I imagine the stack would implement: > > 1. new header flag: skb->newmem > 2. > > static inline bool skb_frags_not_readable(const struct skb_buff *skb) > { > return skb->devmem || skb->newmem; > } > > 3. tcp_recvmsg_devmem() would handle skb->devmem skbs is in this patch > series, but tcp_recvmsg_newmem() would handle skb->newmem skbs.

You copy it to the userspace in a special way because your frags are page_is_page_pool_iov(). I agree with David, the skb bit is just and optimization.

For most of the core stack, it doesn't matter why your skb is not readable. For a few places where it matters (recvmsg?), you can double-check your frags (all or some) with page_is_page_pool_iov.

I see, we can do that then. I.e. make the header flag 'not_readable' and check the frags to decide to delegate to tcp_recvmsg_devmem() or something else. We can even assume not_readable == devmem because currently devmem is the only type of unreadable frag currently.

...

Unrelated: we probably need socket to dmabuf association as well (via netlink or something).

Not sure this is possible. The dma-buf is bound to the rx-queue, and any packets that land on that rx-queue are bound to that dma-buf, regardless of which socket that packet belongs to. So the association IMO must be rx-queue to dma-buf, not socket to dma-buf.

But there is still always 1 dmabuf to 1 socket association (on rx), right? Because otherwise, there is no way currently to tell, at recvmsg, which dmabuf the received token belongs to.

Yes, but this 1 dma-buf to 1 socket association happens because the user binds the dma-buf to an rx-queue and configures flow steering of the socket to that rx-queue.

It's still fixed and won't change during the socket lifetime, right? And the socket has to know this association; otherwise those tokens are useless since they don't carry anything to identify the dmabuf.

I think my other issue with MSG_SOCK_DEVMEM being on recvmsg is that it somehow implies that I have an option of passing or not passing it for an individual system call. If we know that we're going to use dmabuf with the socket, maybe we should move this flag to the socket() syscall?

fd = socket(AF_INET6, SOCK_STREAM, SOCK_DEVMEM);

?

I think it should then be a setsockopt called before any data is exchanged, with no change of modifying mode later. We generally use setsockopts for the mode of a socket. This use of the protocol field in socket() for setting a mode would be novel. Also, it might miss passively opened connections, or be overly restrictive: one approach for all accepted child sockets.

I was thinking this is similar to SOCK_CLOEXEC or SOCK_NONBLOCK? There are plenty of bits we can grab. But setsockopt works as well!