RE: [PATCH net] ice: fix memory leak of aRFS after resuming from suspend

-----Original Message-----
From: Creeley, Brett <redacted>
Sent: Friday, March 19, 2021 06:20
To: Liu, Yongxin <redacted>; jeffrey.t.kirsher@intel.com;
Chittim, Madhu [off-list ref]; Nguyen, Anthony L
[off-list ref]; andrewx.bowers@intel.com
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH net] ice: fix memory leak of aRFS after resuming from
suspend


On Thu, 2021-03-18 at 16:15 +0800, Yongxin Liu wrote:

quoted

In ice_suspend(), ice_clear_interrupt_scheme() is called, and then
irq_free_descs() will be eventually called to free irq and its
descriptor.

In ice_resume(), ice_init_interrupt_scheme() is called to allocate new
irqs.
However, in ice_rebuild_arfs(), struct irq_glue and struct cpu_rmap
maybe cannot be freed, if the irqs that released in ice_suspend() were
reassigned to other devices, which makes irq descriptor's
affinity_notify lost.

So move ice_remove_arfs() before ice_clear_interrupt_scheme(), which
can make sure all irq_glue and cpu_rmap can be correctly released
before corresponding irq and descriptor are released.

Fix the following memeory leak.

s/memeory/memory

<snip>

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_arfs.c

b/drivers/net/ethernet/intel/ice/ice_arfs.c
index 6560acd76c94..c748d0a5c7d4 100644

--- a/drivers/net/ethernet/intel/ice/ice_arfs.c
+++ b/drivers/net/ethernet/intel/ice/ice_arfs.c

@@ -654,7 +654,6 @@ void ice_rebuild_arfs(struct ice_pf *pf)
      if (!pf_vsi)
              return;

-     ice_remove_arfs(pf);

This should not be removed. Removing this would break the reset flows
outside of the suspend/remove case.

quoted

      if (ice_set_cpu_rx_rmap(pf_vsi)) {
              dev_err(ice_pf_to_dev(pf), "Failed to rebuild aRFS\n");
              return;

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c

b/drivers/net/ethernet/intel/ice/ice_main.c
index 2c23c8f468a5..dba901bf2b9b 100644

--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c

@@ -4568,6 +4568,9 @@ static int __maybe_unused ice_suspend(struct

device *dev)
                      continue;
              ice_vsi_free_q_vectors(pf->vsi[v]);
      }
+     if (test_bit(ICE_FLAG_FD_ENA, pf->flags)) {
+             ice_remove_arfs(pf);
+     }

Braces aren't needed around a single if statement like this.

Also, I don't think this is the right solution. I think a better approach
would be to call ice_free_rx_cpu_map() here. With this, it seems like no
other changes are necessary. It also isn't necessary to check the
ICE_FLAG_FD_ENA bit with this change.

Thanks for your valuable review. I will send V2.

--Yongxin

quoted

      ice_clear_interrupt_scheme(pf);

      pci_save_state(pdev);