Re: [PATCH iwl-next 1/1] igc: Improve XDP_SETUP_PROG process

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Song Yoong Siang <yoong.siang.song@intel.com> writes:

> Improve XDP_SETUP_PROG process by avoiding unnecessary link down/up event
> and hardware device reset.
>

Some examples of problems that these hardware resets are causing would
be good.

> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
> ---

The duplication of code doesn't look that good. Initialization is
tricky, it seems to make it easy to update one place and forget to
update the other.

A couple of ideas:
 - separate the code into functions that can be used from the "usual"
 igc_open()/igc_close() flow;
 - it seems that igc_close()/igc_open() are too big a hammer for
 installing a new XDP program: what do we really need? (my mental model
 is: 1. stop new traffic from going into any queue; 2. wait for any
 packets "in progress"; 3. install the program; 4. resume operations;
 what else?)

>  drivers/net/ethernet/intel/igc/igc.h      |   2 +
>  drivers/net/ethernet/intel/igc/igc_main.c | 138 ++++++++++++++++++++++
>  drivers/net/ethernet/intel/igc/igc_xdp.c  |   4 +-
>  3 files changed, 142 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index eac0f966e0e4..b1e46fcaae1a 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -341,6 +341,8 @@ void igc_up(struct igc_adapter *adapter);
>  void igc_down(struct igc_adapter *adapter);
>  int igc_open(struct net_device *netdev);
>  int igc_close(struct net_device *netdev);
> +void igc_xdp_open(struct net_device *netdev);
> +void igc_xdp_close(struct net_device *netdev);
>  int igc_setup_tx_resources(struct igc_ring *ring);
>  int igc_setup_rx_resources(struct igc_ring *ring);
>  void igc_free_tx_resources(struct igc_ring *ring);
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 27872bdea9bd..098529a80b88 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6145,6 +6145,144 @@ int igc_close(struct net_device *netdev)
>  	return 0;
>  }
>  
> +void igc_xdp_open(struct net_device *netdev)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct igc_hw *hw = &adapter->hw;
> +	int err = 0;
> +	int i = 0;
> +
> +	/* disallow open during test */
> +	if (test_bit(__IGC_TESTING, &adapter->state))
> +		return;
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +
> +	igc_ptp_reset(adapter);
> +
> +	/* allocate transmit descriptors */
> +	err = igc_setup_all_tx_resources(adapter);
> +	if (err)
> +		goto err_setup_tx;
> +
> +	/* allocate receive descriptors */
> +	err = igc_setup_all_rx_resources(adapter);
> +	if (err)
> +		goto err_setup_rx;
> +
> +	igc_setup_tctl(adapter);
> +	igc_setup_rctl(adapter);
> +	igc_configure_tx(adapter);
> +	igc_configure_rx(adapter);
> +	igc_rx_fifo_flush_base(&adapter->hw);
> +
> +	/* call igc_desc_unused which always leaves
> +	 * at least 1 descriptor unused to make sure
> +	 * next_to_use != next_to_clean
> +	 */
> +	for (i = 0; i < adapter->num_rx_queues; i++) {
> +		struct igc_ring *ring = adapter->rx_ring[i];
> +
> +		if (ring->xsk_pool)
> +			igc_alloc_rx_buffers_zc(ring, igc_desc_unused(ring));
> +		else
> +			igc_alloc_rx_buffers(ring, igc_desc_unused(ring));
> +	}
> +
> +	err = igc_request_irq(adapter);
> +	if (err)
> +		goto err_req_irq;
> +
> +	clear_bit(__IGC_DOWN, &adapter->state);
> +
> +	for (i = 0; i < adapter->num_q_vectors; i++)
> +		napi_enable(&adapter->q_vector[i]->napi);
> +
> +	/* Clear any pending interrupts. */
> +	rd32(IGC_ICR);
> +	igc_irq_enable(adapter);
> +
> +	pm_runtime_put(&pdev->dev);
> +
> +	netif_tx_start_all_queues(netdev);
> +	netif_carrier_on(netdev);
> +
> +	return;
> +
> +err_req_irq:
> +	igc_release_hw_control(adapter);
> +	igc_power_down_phy_copper_base(&adapter->hw);
> +	igc_free_all_rx_resources(adapter);
> +err_setup_rx:
> +	igc_free_all_tx_resources(adapter);
> +err_setup_tx:
> +	igc_reset(adapter);
> +	pm_runtime_put(&pdev->dev);
> +}
> +
> +void igc_xdp_close(struct net_device *netdev)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 tctl, rctl;
> +	int i = 0;
> +
> +	WARN_ON(test_bit(__IGC_RESETTING, &adapter->state));
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +
> +	set_bit(__IGC_DOWN, &adapter->state);
> +
> +	igc_ptp_suspend(adapter);
> +
> +	if (pci_device_is_present(pdev)) {
> +		/* disable receives in the hardware */
> +		rctl = rd32(IGC_RCTL);
> +		wr32(IGC_RCTL, rctl & ~IGC_RCTL_EN);
> +		/* flush and sleep below */
> +	}
> +	/* set trans_start so we don't get spurious watchdogs during reset */
> +	netif_trans_update(netdev);
> +
> +	netif_carrier_off(netdev);
> +	netif_tx_stop_all_queues(netdev);
> +
> +	if (pci_device_is_present(pdev)) {
> +		/* disable transmits in the hardware */
> +		tctl = rd32(IGC_TCTL);
> +		tctl &= ~IGC_TCTL_EN;
> +		wr32(IGC_TCTL, tctl);
> +		/* flush both disables and wait for them to finish */
> +		wrfl();
> +		usleep_range(10000, 20000);
> +
> +		igc_irq_disable(adapter);
> +	}
> +
> +	for (i = 0; i < adapter->num_q_vectors; i++) {
> +		if (adapter->q_vector[i]) {
> +			napi_synchronize(&adapter->q_vector[i]->napi);
> +			napi_disable(&adapter->q_vector[i]->napi);
> +		}
> +	}
> +
> +	del_timer_sync(&adapter->watchdog_timer);
> +	del_timer_sync(&adapter->phy_info_timer);
> +
> +	igc_disable_all_tx_rings_hw(adapter);
> +	igc_clean_all_tx_rings(adapter);
> +	igc_clean_all_rx_rings(adapter);
> +
> +	igc_free_irq(adapter);
> +
> +	igc_free_all_tx_resources(adapter);
> +	igc_free_all_rx_resources(adapter);
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +}
> +
>  /**
>   * igc_ioctl - Access the hwtstamp interface
>   * @netdev: network interface device structure
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
> index 869815f48ac1..f1d6ab56ab54 100644
> --- a/drivers/net/ethernet/intel/igc/igc_xdp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> @@ -25,7 +25,7 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
>  
>  	need_update = !!adapter->xdp_prog != !!prog;
>  	if (if_running && need_update)
> -		igc_close(dev);
> +		igc_xdp_close(dev);
>  
>  	old_prog = xchg(&adapter->xdp_prog, prog);
>  	if (old_prog)
> @@ -37,7 +37,7 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
>  		xdp_features_clear_redirect_target(dev);
>  
>  	if (if_running && need_update)
> -		igc_open(dev);
> +		igc_xdp_open(dev);
>  
>  	return 0;
>  }
> -- 
> 2.34.1
>
>

Cheers,
-- 
Vinicius