| Message ID | 20241204120233.3148482-1-yoong.siang.song@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-next,1/1] igc: Improve XDP_SETUP_PROG process | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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,
On Thursday, December 5, 2024 8:38 AM, Gomes, Vinicius <vinicius.gomes@intel.com> wrote: >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. > Sure, maybe I can give an example of "before" and "after" behavior on ptp4l clock sync log in commit msg in next submission. >> 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. > The original idea of this patch is to avoid huge changes on igc_open/igc_close functions. But I agree with you that duplication of code is challenging to maintain. I will create common function whenever necessary in next submission. >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?) I think will need to reallocate DMA resource, if user enable XDP pool first, then only setup XDP prog. Maybe I can reuse the reset sequence in igc_xdp_enable_pool(). I will study further. [...] > >Cheers, >-- >Vinicius Thanks & Regards Siang
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; }
Improve XDP_SETUP_PROG process by avoiding unnecessary link down/up event and hardware device reset. Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> --- 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(-)