Re: [PATCH net-next 1/4] igb: prepare for AF_XDP zero-copy support

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Thu, Aug 08, 2024 at 11:35:51AM -0700, Tony Nguyen wrote:
> From: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> 
> Always call igb_xdp_ring_update_tail under __netif_tx_lock, add a
> comment to indicate that. This is needed to share the same TX ring
> between XDP, XSK and slow paths.

standalone commit

> 
> Remove static qualifiers on the following functions to be able to call
> from XSK specific file that is added in the later patches
> - igb_xdp_tx_queue_mapping
> - igb_xdp_ring_update_tail
> - igb_clean_tx_ring
> - igb_clean_rx_ring
> - igb_run_xdp
> - igb_process_skb_fields

ditto

> 
> Introduce igb_xdp_is_enabled() to check if an XDP program is assigned to
> the device.

ditto

> 
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/igb/igb.h      | 15 ++++++++++++
>  drivers/net/ethernet/intel/igb/igb_main.c | 29 +++++++++++------------
>  2 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 3c2dc7bdebb5..0de71ec324ed 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -718,6 +718,8 @@ extern char igb_driver_name[];
>  int igb_xmit_xdp_ring(struct igb_adapter *adapter,
>  		      struct igb_ring *ring,
>  		      struct xdp_frame *xdpf);
> +struct igb_ring *igb_xdp_tx_queue_mapping(struct igb_adapter *adapter);
> +void igb_xdp_ring_update_tail(struct igb_ring *ring);
>  int igb_open(struct net_device *netdev);
>  int igb_close(struct net_device *netdev);
>  int igb_up(struct igb_adapter *);
> @@ -731,12 +733,20 @@ int igb_setup_tx_resources(struct igb_ring *);
>  int igb_setup_rx_resources(struct igb_ring *);
>  void igb_free_tx_resources(struct igb_ring *);
>  void igb_free_rx_resources(struct igb_ring *);
> +void igb_clean_tx_ring(struct igb_ring *tx_ring);
> +void igb_clean_rx_ring(struct igb_ring *rx_ring);
>  void igb_configure_tx_ring(struct igb_adapter *, struct igb_ring *);
>  void igb_configure_rx_ring(struct igb_adapter *, struct igb_ring *);
>  void igb_setup_tctl(struct igb_adapter *);
>  void igb_setup_rctl(struct igb_adapter *);
>  void igb_setup_srrctl(struct igb_adapter *, struct igb_ring *);
>  netdev_tx_t igb_xmit_frame_ring(struct sk_buff *, struct igb_ring *);
> +struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,
> +			    struct igb_ring *rx_ring,
> +			    struct xdp_buff *xdp);
> +void igb_process_skb_fields(struct igb_ring *rx_ring,
> +			    union e1000_adv_rx_desc *rx_desc,
> +			    struct sk_buff *skb);
>  void igb_alloc_rx_buffers(struct igb_ring *, u16);
>  void igb_update_stats(struct igb_adapter *);
>  bool igb_has_link(struct igb_adapter *adapter);
> @@ -797,6 +807,11 @@ static inline struct netdev_queue *txring_txq(const struct igb_ring *tx_ring)
>  	return netdev_get_tx_queue(tx_ring->netdev, tx_ring->queue_index);
>  }
>  
> +static inline bool igb_xdp_is_enabled(struct igb_adapter *adapter)
> +{
> +	return !!adapter->xdp_prog;

READ_ONCE() plus use this everywhere else where prog is read.

> +}
> +
>  int igb_add_filter(struct igb_adapter *adapter,
>  		   struct igb_nfc_filter *input);
>  int igb_erase_filter(struct igb_adapter *adapter,
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 11be39f435f3..bdb7637559b8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -115,8 +115,6 @@ static void igb_configure_tx(struct igb_adapter *);
>  static void igb_configure_rx(struct igb_adapter *);
>  static void igb_clean_all_tx_rings(struct igb_adapter *);
>  static void igb_clean_all_rx_rings(struct igb_adapter *);
> -static void igb_clean_tx_ring(struct igb_ring *);
> -static void igb_clean_rx_ring(struct igb_ring *);
>  static void igb_set_rx_mode(struct net_device *);
>  static void igb_update_phy_info(struct timer_list *);
>  static void igb_watchdog(struct timer_list *);
> @@ -2914,7 +2912,8 @@ static int igb_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>  	}
>  }
>  
> -static void igb_xdp_ring_update_tail(struct igb_ring *ring)
> +/* This function assumes __netif_tx_lock is held by the caller. */
> +void igb_xdp_ring_update_tail(struct igb_ring *ring)
>  {
>  	/* Force memory writes to complete before letting h/w know there
>  	 * are new descriptors to fetch.
> @@ -2923,7 +2922,7 @@ static void igb_xdp_ring_update_tail(struct igb_ring *ring)
>  	writel(ring->next_to_use, ring->tail);
>  }
>  
> -static struct igb_ring *igb_xdp_tx_queue_mapping(struct igb_adapter *adapter)
> +struct igb_ring *igb_xdp_tx_queue_mapping(struct igb_adapter *adapter)
>  {
>  	unsigned int r_idx = smp_processor_id();
>  
> @@ -3000,11 +2999,11 @@ static int igb_xdp_xmit(struct net_device *dev, int n,
>  		nxmit++;
>  	}
>  
> -	__netif_tx_unlock(nq);
> -
>  	if (unlikely(flags & XDP_XMIT_FLUSH))
>  		igb_xdp_ring_update_tail(tx_ring);
>  
> +	__netif_tx_unlock(nq);
> +
>  	return nxmit;
>  }
>  
> @@ -4879,7 +4878,7 @@ static void igb_free_all_tx_resources(struct igb_adapter *adapter)
>   *  igb_clean_tx_ring - Free Tx Buffers
>   *  @tx_ring: ring to be cleaned
>   **/
> -static void igb_clean_tx_ring(struct igb_ring *tx_ring)
> +void igb_clean_tx_ring(struct igb_ring *tx_ring)
>  {
>  	u16 i = tx_ring->next_to_clean;
>  	struct igb_tx_buffer *tx_buffer = &tx_ring->tx_buffer_info[i];
> @@ -4998,7 +4997,7 @@ static void igb_free_all_rx_resources(struct igb_adapter *adapter)
>   *  igb_clean_rx_ring - Free Rx Buffers per Queue
>   *  @rx_ring: ring to free buffers from
>   **/
> -static void igb_clean_rx_ring(struct igb_ring *rx_ring)
> +void igb_clean_rx_ring(struct igb_ring *rx_ring)
>  {
>  	u16 i = rx_ring->next_to_clean;
>  
> @@ -6613,7 +6612,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
>  	struct igb_adapter *adapter = netdev_priv(netdev);
>  	int max_frame = new_mtu + IGB_ETH_PKT_HDR_PAD;
>  
> -	if (adapter->xdp_prog) {
> +	if (igb_xdp_is_enabled(adapter)) {
>  		int i;
>  
>  		for (i = 0; i < adapter->num_rx_queues; i++) {
> @@ -8569,9 +8568,9 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
>  	return skb;
>  }
>  
> -static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,
> -				   struct igb_ring *rx_ring,
> -				   struct xdp_buff *xdp)
> +struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,
> +			    struct igb_ring *rx_ring,
> +			    struct xdp_buff *xdp)
>  {
>  	int err, result = IGB_XDP_PASS;
>  	struct bpf_prog *xdp_prog;
> @@ -8767,9 +8766,9 @@ static bool igb_cleanup_headers(struct igb_ring *rx_ring,
>   *  order to populate the hash, checksum, VLAN, timestamp, protocol, and
>   *  other fields within the skb.
>   **/
> -static void igb_process_skb_fields(struct igb_ring *rx_ring,
> -				   union e1000_adv_rx_desc *rx_desc,
> -				   struct sk_buff *skb)
> +void igb_process_skb_fields(struct igb_ring *rx_ring,
> +			    union e1000_adv_rx_desc *rx_desc,
> +			    struct sk_buff *skb)
>  {
>  	struct net_device *dev = rx_ring->netdev;
>  
> -- 
> 2.42.0
> 
>