Re: [PATCH bpf-next v4 05/14] xsk: move queue_id, dev and need_wakeup to buffer pool

On Tue, Jul 28, 2020 at 9:10 AM Björn Töpel [off-list ref] wrote:

On 2020-07-21 07:03, Magnus Karlsson wrote:

quoted

Move queue_id, dev, and need_wakeup from the umem to the
buffer pool. This so that we in a later commit can share the umem
between multiple HW queues. There is one buffer pool per dev and
queue id, so these variables should belong to the buffer pool, not
the umem. Need_wakeup is also something that is set on a per napi
level, so there is usually one per device and queue id. So move
this to the buffer pool too.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
  include/net/xdp_sock.h      |  3 ---
  include/net/xsk_buff_pool.h |  4 ++++
  net/xdp/xdp_umem.c          | 19 +------------------
  net/xdp/xdp_umem.h          |  4 ----
  net/xdp/xsk.c               | 40 +++++++++++++++-------------------------
  net/xdp/xsk_buff_pool.c     | 39 ++++++++++++++++++++++-----------------
  net/xdp/xsk_diag.c          |  4 ++--
  7 files changed, 44 insertions(+), 69 deletions(-)

[...]

quoted

              }

diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 36287d2..436648a 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c

@@ -95,10 +95,9 @@ void xp_set_rxq_info(struct xsk_buff_pool *pool, struct xdp_rxq_info *rxq)
  }
  EXPORT_SYMBOL(xp_set_rxq_info);

-int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *dev,
+int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *netdev,
                u16 queue_id, u16 flags)
  {
-     struct xdp_umem *umem = pool->umem;
      bool force_zc, force_copy;
      struct netdev_bpf bpf;
      int err = 0;

@@ -111,27 +110,30 @@ int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *dev,
      if (force_zc && force_copy)
              return -EINVAL;

-     if (xsk_get_pool_from_qid(dev, queue_id))
+     if (xsk_get_pool_from_qid(netdev, queue_id))
              return -EBUSY;

-     err = xsk_reg_pool_at_qid(dev, pool, queue_id);
+     err = xsk_reg_pool_at_qid(netdev, pool, queue_id);
      if (err)
              return err;

      if (flags & XDP_USE_NEED_WAKEUP) {
-             umem->flags |= XDP_UMEM_USES_NEED_WAKEUP;
+             pool->uses_need_wakeup = true;
              /* Tx needs to be explicitly woken up the first time.
               * Also for supporting drivers that do not implement this
               * feature. They will always have to call sendto().
               */
-             umem->need_wakeup = XDP_WAKEUP_TX;
+             pool->cached_need_wakeup = XDP_WAKEUP_TX;
      }

+     dev_hold(netdev);
+

You have a reference leak here for the error case.

Thanks. Will fix.

/Magnus

Björn