diff mbox series

[net-next,4/9] iavf: in iavf_down, disable queues when removing the driver

Message ID 20231023230826.531858-6-jacob.e.keller@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Intel Wired LAN Driver Updates for 2023-10-23 (iavf) | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1363 this patch: 1363
netdev/cc_maintainers fail 2 blamed authors not CCed: radoslawx.tyl@intel.com anthony.l.nguyen@intel.com; 6 maintainers not CCed: intel-wired-lan@lists.osuosl.org pabeni@redhat.com edumazet@google.com jesse.brandeburg@intel.com radoslawx.tyl@intel.com anthony.l.nguyen@intel.com
netdev/build_clang success Errors and warnings before: 1388 this patch: 1388
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1388 this patch: 1388
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jacob Keller Oct. 23, 2023, 11:08 p.m. UTC 
From: Michal Schmidt <mschmidt@redhat.com>

In iavf_down, we're skipping the scheduling of certain operations if
the driver is being removed. However, the IAVF_FLAG_AQ_DISABLE_QUEUES
request must not be skipped in this case, because iavf_close waits
for the transition to the __IAVF_DOWN state, which happens in
iavf_virtchnl_completion after the queues are released.

Without this fix, "rmmod iavf" takes half a second per interface that's
up and prints the "Device resources not yet released" warning.

Fixes: c8de44b577eb ("iavf: do not process adminq tasks when __IAVF_IN_REMOVE_TASK is set")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Kicinski Oct. 24, 2023, 11:42 p.m. UTC | #1 
On Mon, 23 Oct 2023 16:08:21 -0700 Jacob Keller wrote:
> From: Michal Schmidt <mschmidt@redhat.com>
> 
> In iavf_down, we're skipping the scheduling of certain operations if
> the driver is being removed. However, the IAVF_FLAG_AQ_DISABLE_QUEUES
> request must not be skipped in this case, because iavf_close waits
> for the transition to the __IAVF_DOWN state, which happens in
> iavf_virtchnl_completion after the queues are released.
> 
> Without this fix, "rmmod iavf" takes half a second per interface that's
> up and prints the "Device resources not yet released" warning.
> 
> Fixes: c8de44b577eb ("iavf: do not process adminq tasks when __IAVF_IN_REMOVE_TASK is set")

This looks like a 6.6 regression, why send it for net-next?
Michal Schmidt Oct. 25, 2023, 3:24 p.m. UTC | #2 
On Wed, Oct 25, 2023 at 1:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 23 Oct 2023 16:08:21 -0700 Jacob Keller wrote:
> > From: Michal Schmidt <mschmidt@redhat.com>
> >
> > In iavf_down, we're skipping the scheduling of certain operations if
> > the driver is being removed. However, the IAVF_FLAG_AQ_DISABLE_QUEUES
> > request must not be skipped in this case, because iavf_close waits
> > for the transition to the __IAVF_DOWN state, which happens in
> > iavf_virtchnl_completion after the queues are released.
> >
> > Without this fix, "rmmod iavf" takes half a second per interface that's
> > up and prints the "Device resources not yet released" warning.
> >
> > Fixes: c8de44b577eb ("iavf: do not process adminq tasks when __IAVF_IN_REMOVE_TASK is set")
>
> This looks like a 6.6 regression, why send it for net-next?

Hi Jakub,
At first I thought I had a dependency on the preceding patch in the
series, but after rethinking and retesting it, it's actually fine to
put this patch in net.git.
Can you please do that, or will you require resending?

Thanks,
Michal
Jakub Kicinski Oct. 25, 2023, 4:25 p.m. UTC | #3 
On Wed, 25 Oct 2023 17:24:59 +0200 Michal Schmidt wrote:
> > This looks like a 6.6 regression, why send it for net-next?  
> 
> Hi Jakub,
> At first I thought I had a dependency on the preceding patch in the
> series, but after rethinking and retesting it, it's actually fine to
> put this patch in net.git.
> Can you please do that, or will you require resending?

I'd prefer if Jake could resend just the fix for net, after re-testing
that it indeed works right. I'll make sure that it makes tomorrow's PR
from net, in case the net-next stuff would conflict.
Jacob Keller Oct. 25, 2023, 5:23 p.m. UTC | #4 
On 10/25/2023 9:25 AM, Jakub Kicinski wrote:
> On Wed, 25 Oct 2023 17:24:59 +0200 Michal Schmidt wrote:
>>> This looks like a 6.6 regression, why send it for net-next?  
>>
>> Hi Jakub,
>> At first I thought I had a dependency on the preceding patch in the
>> series, but after rethinking and retesting it, it's actually fine to
>> put this patch in net.git.
>> Can you please do that, or will you require resending?
> 
> I'd prefer if Jake could resend just the fix for net, after re-testing
> that it indeed works right. I'll make sure that it makes tomorrow's PR
> from net, in case the net-next stuff would conflict.
> 

Will do, thanks.
Jacob Keller Oct. 25, 2023, 6:32 p.m. UTC | #5 
On 10/25/2023 10:23 AM, Jacob Keller wrote:
> 
> 
> On 10/25/2023 9:25 AM, Jakub Kicinski wrote:
>> On Wed, 25 Oct 2023 17:24:59 +0200 Michal Schmidt wrote:
>>>> This looks like a 6.6 regression, why send it for net-next?  
>>>
>>> Hi Jakub,
>>> At first I thought I had a dependency on the preceding patch in the
>>> series, but after rethinking and retesting it, it's actually fine to
>>> put this patch in net.git.
>>> Can you please do that, or will you require resending?
>>
>> I'd prefer if Jake could resend just the fix for net, after re-testing
>> that it indeed works right. I'll make sure that it makes tomorrow's PR
>> from net, in case the net-next stuff would conflict.
>>
> 
> Will do, thanks.
> 

I tested this on one of my dev systems with 128 VFs. Without the fix
applied:

$ time rmmod iavf
real    1m19.132s
user    0m0.007s
sys     0m1.974s

With the fix applied:

$ time rmmod iavf
real    0m17.951s
user    0m0.006s
sys     0m0.827s

I'll send this out to net shortly.

Thanks,
Jake
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index b30d703e26a1..7ca19dfea109 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1440,9 +1440,9 @@  void iavf_down(struct iavf_adapter *adapter)
 			adapter->aq_required |= IAVF_FLAG_AQ_DEL_FDIR_FILTER;
 		if (!list_empty(&adapter->adv_rss_list_head))
 			adapter->aq_required |= IAVF_FLAG_AQ_DEL_ADV_RSS_CFG;
-		adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
 	}
 
+	adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
 	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 }