On Tue, Jul 28, 2020 at 11:22 AM Maxim Mikityanskiy
<maximmi@xxxxxxxxxxxx> wrote:
>
> On 2020-07-21 08:03, Magnus Karlsson wrote:
> > 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@xxxxxxxxx>
> > ---
> > 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(-)
> >
> > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > index 2a284e1..b052f1c 100644
> > --- a/include/net/xdp_sock.h
> > +++ b/include/net/xdp_sock.h
> > @@ -26,11 +26,8 @@ struct xdp_umem {
> > refcount_t users;
> > struct page **pgs;
> > u32 npgs;
> > - u16 queue_id;
> > - u8 need_wakeup;
> > u8 flags;
> > int id;
> > - struct net_device *dev;
> > bool zc;
> > spinlock_t xsk_tx_list_lock;
> > struct list_head xsk_tx_list;
> > diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> > index 380d9ae..2d94890 100644
> > --- a/include/net/xsk_buff_pool.h
> > +++ b/include/net/xsk_buff_pool.h
> > @@ -43,11 +43,15 @@ struct xsk_buff_pool {
> > u32 headroom;
> > u32 chunk_size;
> > u32 frame_len;
> > + u16 queue_id;
> > + u8 cached_need_wakeup;
> > + bool uses_need_wakeup;
> > bool dma_need_sync;
> > bool unaligned;
> > struct xdp_umem *umem;
> > void *addrs;
> > struct device *dev;
> > + struct net_device *netdev;
> > refcount_t users;
> > struct work_struct work;
> > struct xdp_buff_xsk *free_heads[];
> > diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> > index 7d86a63..b1699d0 100644
> > --- a/net/xdp/xdp_umem.c
> > +++ b/net/xdp/xdp_umem.c
> > @@ -63,26 +63,9 @@ static void xdp_umem_unaccount_pages(struct xdp_umem *umem)
> > }
> > }
> >
> > -void xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> > - u16 queue_id)
> > -{
> > - umem->dev = dev;
> > - umem->queue_id = queue_id;
> > -
> > - dev_hold(dev);
> > -}
> > -
> > -void xdp_umem_clear_dev(struct xdp_umem *umem)
> > -{
> > - dev_put(umem->dev);
> > - umem->dev = NULL;
> > - umem->zc = false;
> > -}
> > -
> > static void xdp_umem_release(struct xdp_umem *umem)
> > {
> > - xdp_umem_clear_dev(umem);
> > -
> > + umem->zc = false;
> > ida_simple_remove(&umem_ida, umem->id);
> >
> > xdp_umem_unpin_pages(umem);
> > diff --git a/net/xdp/xdp_umem.h b/net/xdp/xdp_umem.h
> > index 93e96be..67bf3f3 100644
> > --- a/net/xdp/xdp_umem.h
> > +++ b/net/xdp/xdp_umem.h
> > @@ -8,10 +8,6 @@
> >
> > #include <net/xdp_sock_drv.h>
> >
> > -void xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> > - u16 queue_id);
> > -void xdp_umem_clear_dev(struct xdp_umem *umem);
> > -bool xdp_umem_validate_queues(struct xdp_umem *umem);
> > void xdp_get_umem(struct xdp_umem *umem);
> > void xdp_put_umem(struct xdp_umem *umem);
> > void xdp_add_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs);
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index ee04887..624d0fc 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -41,67 +41,61 @@ bool xsk_is_setup_for_bpf_map(struct xdp_sock *xs)
> >
> > void xsk_set_rx_need_wakeup(struct xsk_buff_pool *pool)
> > {
> > - struct xdp_umem *umem = pool->umem;
> > -
> > - if (umem->need_wakeup & XDP_WAKEUP_RX)
> > + if (pool->cached_need_wakeup & XDP_WAKEUP_RX)
> > return;
> >
> > pool->fq->ring->flags |= XDP_RING_NEED_WAKEUP;
> > - umem->need_wakeup |= XDP_WAKEUP_RX;
> > + pool->cached_need_wakeup |= XDP_WAKEUP_RX;
> > }
> > EXPORT_SYMBOL(xsk_set_rx_need_wakeup);
> >
> > void xsk_set_tx_need_wakeup(struct xsk_buff_pool *pool)
> > {
> > - struct xdp_umem *umem = pool->umem;
> > struct xdp_sock *xs;
> >
> > - if (umem->need_wakeup & XDP_WAKEUP_TX)
> > + if (pool->cached_need_wakeup & XDP_WAKEUP_TX)
> > return;
> >
> > rcu_read_lock();
> > - list_for_each_entry_rcu(xs, &umem->xsk_tx_list, list) {
> > + list_for_each_entry_rcu(xs, &xs->umem->xsk_tx_list, list) {
>
> I know this is going to be fixed in the next patch, but it breaks
> bisectability: this patch is broken because xs is not assigned. I don't
> think this change is actually needed here, it doesn't belong to this
> patch, and you can convert it to &pool->xsk_tx_list in one step in the
> next patch.
Thanks. Will fix this and the other comment you had below.
/Magnus
> > xs->tx->ring->flags |= XDP_RING_NEED_WAKEUP;
> > }
> > rcu_read_unlock();
> >
> > - umem->need_wakeup |= XDP_WAKEUP_TX;
> > + pool->cached_need_wakeup |= XDP_WAKEUP_TX;
> > }
> > EXPORT_SYMBOL(xsk_set_tx_need_wakeup);
> >
> > void xsk_clear_rx_need_wakeup(struct xsk_buff_pool *pool)
> > {
> > - struct xdp_umem *umem = pool->umem;
> > -
> > - if (!(umem->need_wakeup & XDP_WAKEUP_RX))
> > + if (!(pool->cached_need_wakeup & XDP_WAKEUP_RX))
> > return;
> >
> > pool->fq->ring->flags &= ~XDP_RING_NEED_WAKEUP;
> > - umem->need_wakeup &= ~XDP_WAKEUP_RX;
> > + pool->cached_need_wakeup &= ~XDP_WAKEUP_RX;
> > }
> > EXPORT_SYMBOL(xsk_clear_rx_need_wakeup);
> >
> > void xsk_clear_tx_need_wakeup(struct xsk_buff_pool *pool)
> > {
> > - struct xdp_umem *umem = pool->umem;
> > struct xdp_sock *xs;
> >
> > - if (!(umem->need_wakeup & XDP_WAKEUP_TX))
> > + if (!(pool->cached_need_wakeup & XDP_WAKEUP_TX))
> > return;
> >
> > rcu_read_lock();
> > - list_for_each_entry_rcu(xs, &umem->xsk_tx_list, list) {
> > + list_for_each_entry_rcu(xs, &xs->umem->xsk_tx_list, list) {
>
> Same here.
>
> > xs->tx->ring->flags &= ~XDP_RING_NEED_WAKEUP;
> > }
> > rcu_read_unlock();
> >
> > - umem->need_wakeup &= ~XDP_WAKEUP_TX;
> > + pool->cached_need_wakeup &= ~XDP_WAKEUP_TX;
> > }
> > EXPORT_SYMBOL(xsk_clear_tx_need_wakeup);
> >
> > bool xsk_uses_need_wakeup(struct xsk_buff_pool *pool)
> > {
> > - return pool->umem->flags & XDP_UMEM_USES_NEED_WAKEUP;
> > + return pool->uses_need_wakeup;
> > }
> > EXPORT_SYMBOL(xsk_uses_need_wakeup);
> >
> > @@ -478,16 +472,16 @@ static __poll_t xsk_poll(struct file *file, struct socket *sock,
> > __poll_t mask = datagram_poll(file, sock, wait);
> > struct sock *sk = sock->sk;
> > struct xdp_sock *xs = xdp_sk(sk);
> > - struct xdp_umem *umem;
> > + struct xsk_buff_pool *pool;
> >
> > if (unlikely(!xsk_is_bound(xs)))
> > return mask;
> >
> > - umem = xs->umem;
> > + pool = xs->pool;
> >
> > - if (umem->need_wakeup) {
> > + if (pool->cached_need_wakeup) {
> > if (xs->zc)
> > - xsk_wakeup(xs, umem->need_wakeup);
> > + xsk_wakeup(xs, pool->cached_need_wakeup);
> > else
> > /* Poll needs to drive Tx also in copy mode */
> > __xsk_sendmsg(sk);
> > @@ -731,11 +725,9 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> > goto out_unlock;
> > } else {
> > /* This xsk has its own umem. */
> > - xdp_umem_assign_dev(xs->umem, dev, qid);
> > xs->pool = xp_create_and_assign_umem(xs, xs->umem);
> > if (!xs->pool) {
> > err = -ENOMEM;
> > - xdp_umem_clear_dev(xs->umem);
> > goto out_unlock;
> > }
> >
> > @@ -743,7 +735,6 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> > if (err) {
> > xp_destroy(xs->pool);
> > xs->pool = NULL;
> > - xdp_umem_clear_dev(xs->umem);
> > goto out_unlock;
> > }
> > }
> > @@ -1089,7 +1080,6 @@ static int xsk_notifier(struct notifier_block *this,
> >
> > /* Clear device references. */
> > xp_clear_dev(xs->pool);
> > - xdp_umem_clear_dev(xs->umem);
> > }
> > mutex_unlock(&xs->mutex);
> > }
> > 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;
>
> XDP_UMEM_USES_NEED_WAKEUP definition should be removed, along with its
> remaining usage in xdp_umem_reg.
>
> > + 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);
> > +
> > if (force_copy)
> > /* For copy-mode, we are done. */
> > return 0;
> >
> > - if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_wakeup) {
> > + if (!netdev->netdev_ops->ndo_bpf ||
> > + !netdev->netdev_ops->ndo_xsk_wakeup) {
> > err = -EOPNOTSUPP;
> > goto err_unreg_pool;
> > }
> > @@ -140,44 +142,47 @@ int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *dev,
> > bpf.xsk.pool = pool;
> > bpf.xsk.queue_id = queue_id;
> >
> > - err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> > + err = netdev->netdev_ops->ndo_bpf(netdev, &bpf);
> > if (err)
> > goto err_unreg_pool;
> >
> > - umem->zc = true;
> > + pool->netdev = netdev;
> > + pool->queue_id = queue_id;
> > + pool->umem->zc = true;
> > return 0;
> >
> > err_unreg_pool:
> > if (!force_zc)
> > err = 0; /* fallback to copy mode */
> > if (err)
> > - xsk_clear_pool_at_qid(dev, queue_id);
> > + xsk_clear_pool_at_qid(netdev, queue_id);
> > return err;
> > }
> >
> > void xp_clear_dev(struct xsk_buff_pool *pool)
> > {
> > - struct xdp_umem *umem = pool->umem;
> > struct netdev_bpf bpf;
> > int err;
> >
> > ASSERT_RTNL();
> >
> > - if (!umem->dev)
> > + if (!pool->netdev)
> > return;
> >
> > - if (umem->zc) {
> > + if (pool->umem->zc) {
> > bpf.command = XDP_SETUP_XSK_POOL;
> > bpf.xsk.pool = NULL;
> > - bpf.xsk.queue_id = umem->queue_id;
> > + bpf.xsk.queue_id = pool->queue_id;
> >
> > - err = umem->dev->netdev_ops->ndo_bpf(umem->dev, &bpf);
> > + err = pool->netdev->netdev_ops->ndo_bpf(pool->netdev, &bpf);
> >
> > if (err)
> > - WARN(1, "failed to disable umem!\n");
> > + WARN(1, "Failed to disable zero-copy!\n");
> > }
> >
> > - xsk_clear_pool_at_qid(umem->dev, umem->queue_id);
> > + xsk_clear_pool_at_qid(pool->netdev, pool->queue_id);
> > + dev_put(pool->netdev);
> > + pool->netdev = NULL;
> > }
> >
> > static void xp_release_deferred(struct work_struct *work)
> > diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
> > index 52675ea..5bd8ea9 100644
> > --- a/net/xdp/xsk_diag.c
> > +++ b/net/xdp/xsk_diag.c
> > @@ -59,8 +59,8 @@ static int xsk_diag_put_umem(const struct xdp_sock *xs, struct sk_buff *nlskb)
> > du.num_pages = umem->npgs;
> > du.chunk_size = umem->chunk_size;
> > du.headroom = umem->headroom;
> > - du.ifindex = umem->dev ? umem->dev->ifindex : 0;
> > - du.queue_id = umem->queue_id;
> > + du.ifindex = pool->netdev ? pool->netdev->ifindex : 0;
> > + du.queue_id = pool->queue_id;
> > du.flags = 0;
> > if (umem->zc)
> > du.flags |= XDP_DU_F_ZEROCOPY;
> >
>
