> -----Original Message-----
> From: Creeley, Brett <brett.cree...@intel.com>
> Sent: Friday, March 19, 2021 06:20
> To: Liu, Yongxin <yongxin....@windriver.com>; jeffrey.t.kirs...@intel.com;
> Chittim, Madhu <madhu.chit...@intel.com>; Nguyen, Anthony L
> <anthony.l.ngu...@intel.com>; andrewx.bow...@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:
> > 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>
>
> > 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.
>
> > 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
>
> > ice_clear_interrupt_scheme(pf);
> >
> > pci_save_state(pdev);
