* [PATCH iwl-next 1/1] igc: Improve XDP_SETUP_PROG process
@ 2024-12-04 12:02 Song Yoong Siang
2024-12-05 0:38 ` Vinicius Costa Gomes
0 siblings, 1 reply; 3+ messages in thread
From: Song Yoong Siang @ 2024-12-04 12:02 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, David S . Miller,
Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
Tony Nguyen, Przemek Kitszel, Andrew Lunn, Eric Dumazet,
Paolo Abeni, Vinicius Costa Gomes, Richard Cochran
Cc: intel-wired-lan, netdev, linux-kernel, bpf
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH iwl-next 1/1] igc: Improve XDP_SETUP_PROG process
2024-12-04 12:02 [PATCH iwl-next 1/1] igc: Improve XDP_SETUP_PROG process Song Yoong Siang
@ 2024-12-05 0:38 ` Vinicius Costa Gomes
2024-12-05 2:45 ` Song, Yoong Siang
0 siblings, 1 reply; 3+ messages in thread
From: Vinicius Costa Gomes @ 2024-12-05 0:38 UTC (permalink / raw)
To: Song Yoong Siang, Alexei Starovoitov, Daniel Borkmann,
David S . Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
Eric Dumazet, Paolo Abeni, Richard Cochran
Cc: intel-wired-lan, netdev, linux-kernel, bpf
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH iwl-next 1/1] igc: Improve XDP_SETUP_PROG process
2024-12-05 0:38 ` Vinicius Costa Gomes
@ 2024-12-05 2:45 ` Song, Yoong Siang
0 siblings, 0 replies; 3+ messages in thread
From: Song, Yoong Siang @ 2024-12-05 2:45 UTC (permalink / raw)
To: Gomes, Vinicius, Alexei Starovoitov, Daniel Borkmann,
David S . Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Nguyen, Anthony L, Kitszel, Przemyslaw,
Andrew Lunn, Eric Dumazet, Paolo Abeni, Richard Cochran
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bpf@vger.kernel.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-12-05 2:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-04 12:02 [PATCH iwl-next 1/1] igc: Improve XDP_SETUP_PROG process Song Yoong Siang
2024-12-05 0:38 ` Vinicius Costa Gomes
2024-12-05 2:45 ` Song, Yoong Siang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).