Re: [PATCH net-next 13/20] xsk: Proxy pool management for mapped queues

[Stanislav Fomichev <stfomichev@gmail.com>]

On 09/19, Daniel Borkmann wrote:
> Similarly what we do for net_mp_{open,close}_rxq for mapped queues,
> proxy also the xsk_{reg,clear}_pool_at_qid via __netif_get_rx_queue_peer
> such that when a virtual netdev picked a mapped rxq, the request gets
> through to the real rxq in the physical netdev.
> 
> Change the function signatures for queue_id to unsigned int in order
> to pass the queue_id parameter into __netif_get_rx_queue_peer. The
> proxying is only relevant for queue_id < dev->real_num_rx_queues since
> right now its only supported for rxqs.
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Co-developed-by: David Wei <dw@davidwei.uk>
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  include/net/xdp_sock_drv.h |  4 ++--
>  net/xdp/xsk.c              | 16 +++++++++++-----
>  net/xdp/xsk.h              |  5 ++---
>  3 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> index 47120666d8d6..709af292cba7 100644
> --- a/include/net/xdp_sock_drv.h
> +++ b/include/net/xdp_sock_drv.h
> @@ -29,7 +29,7 @@ bool xsk_tx_peek_desc(struct xsk_buff_pool *pool, struct xdp_desc *desc);
>  u32 xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, u32 max);
>  void xsk_tx_release(struct xsk_buff_pool *pool);
>  struct xsk_buff_pool *xsk_get_pool_from_qid(struct net_device *dev,
> -					    u16 queue_id);
> +					    unsigned int queue_id);
>  void xsk_set_rx_need_wakeup(struct xsk_buff_pool *pool);
>  void xsk_set_tx_need_wakeup(struct xsk_buff_pool *pool);
>  void xsk_clear_rx_need_wakeup(struct xsk_buff_pool *pool);
> @@ -286,7 +286,7 @@ static inline void xsk_tx_release(struct xsk_buff_pool *pool)
>  }
>  
>  static inline struct xsk_buff_pool *
> -xsk_get_pool_from_qid(struct net_device *dev, u16 queue_id)
> +xsk_get_pool_from_qid(struct net_device *dev, unsigned int queue_id)
>  {
>  	return NULL;
>  }
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index cf40c70ee59f..b9efa6d8a112 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -23,6 +23,8 @@
>  #include <linux/netdevice.h>
>  #include <linux/rculist.h>
>  #include <linux/vmalloc.h>
> +
> +#include <net/netdev_queues.h>
>  #include <net/xdp_sock_drv.h>
>  #include <net/busy_poll.h>
>  #include <net/netdev_lock.h>
> @@ -111,19 +113,20 @@ bool xsk_uses_need_wakeup(struct xsk_buff_pool *pool)
>  EXPORT_SYMBOL(xsk_uses_need_wakeup);
>  
>  struct xsk_buff_pool *xsk_get_pool_from_qid(struct net_device *dev,
> -					    u16 queue_id)
> +					    unsigned int queue_id)
>  {
>  	if (queue_id < dev->real_num_rx_queues)
>  		return dev->_rx[queue_id].pool;
>  	if (queue_id < dev->real_num_tx_queues)
>  		return dev->_tx[queue_id].pool;
> -
>  	return NULL;
>  }
>  EXPORT_SYMBOL(xsk_get_pool_from_qid);
>  
> -void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id)
> +void xsk_clear_pool_at_qid(struct net_device *dev, unsigned int queue_id)
>  {
> +	if (queue_id < dev->real_num_rx_queues)
> +		__netif_get_rx_queue_peer(&dev, &queue_id);
>  	if (queue_id < dev->num_rx_queues)
>  		dev->_rx[queue_id].pool = NULL;
>  	if (queue_id < dev->num_tx_queues)
> @@ -135,7 +138,7 @@ void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id)
>   * This might also change during run time.
>   */
>  int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool,
> -			u16 queue_id)
> +			unsigned int queue_id)
>  {
>  	if (queue_id >= max_t(unsigned int,
>  			      dev->real_num_rx_queues,
> @@ -143,6 +146,10 @@ int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool,
>  		return -EINVAL;
>  	if (xsk_get_pool_from_qid(dev, queue_id))
>  		return -EBUSY;
> +	if (queue_id < dev->real_num_rx_queues)
> +		__netif_get_rx_queue_peer(&dev, &queue_id);
> +	if (xsk_get_pool_from_qid(dev, queue_id))
> +		return -EBUSY;
>  
>  	pool->netdev = dev;
>  	pool->queue_id = queue_id;

I feel like both of the above are also gonna be problematic wrt netdev
lock. The callers lock the netdev, the callers will also have
to resolve the virtual->real queue mapping. Hacking up the
queue/netdev deep in the call stack in a few places is not gonna work.

Maybe also add assert for the (new) netdev lock to __netif_get_rx_queue_peer
to trigger these.