From mboxrd@z Thu Jan 1 00:00:00 1970 From: Song Liu Subject: Re: pegged softirq and NAPI race (?) Date: Tue, 18 Sep 2018 21:40:56 +0000 Message-ID: References: <0FD562CC-CDE9-43C8-9623-B42AC7A208C8@fb.com> <324c4a85-1749-9129-bf03-73a49d9a4059@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Eric Dumazet , Eric Dumazet , Alexei Starovoitov , netdev , "Alexander Duyck" , "michael.chan@broadcom.com" , Kernel Team To: "jeffrey.t.kirsher@intel.com" Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:58052 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730225AbeISDPe (ORCPT ); Tue, 18 Sep 2018 23:15:34 -0400 In-Reply-To: Content-Language: en-US Content-ID: <98AAEF1D8D9CAD4488DCC4E5D6A9DC7B@namprd15.prod.outlook.com> Sender: netdev-owner@vger.kernel.org List-ID: > On Sep 18, 2018, at 2:36 PM, Jeff Kirsher w= rote: >=20 > On Tue, 2018-09-18 at 14:21 -0700, Eric Dumazet wrote: >>=20 >> On 09/18/2018 02:13 PM, Eric Dumazet wrote: >>> On Tue, Sep 18, 2018 at 1:37 PM Song Liu >>> wrote: >>>>=20 >>>> Looks like a patch like the following fixes the issue for ixgbe. >>>> But I >>>> cannot explain it yet. >>>>=20 >>>> Does this ring a bell? >>>=20 >>> I dunno, it looks like the NIC is generating an interrupt while it >>> should not, >>> and constantly sets NAPI_STATE_MISSED. >>>=20 >>> Or maybe we need to properly check napi_complete_done() return >>> value. >>>=20 >>> diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c >>> b/drivers/net/ethernet/intel/ixgb/ixgb_main.c >>> index >>> d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedc >>> dcb72dc1c401813 >>> 100644 >>> --- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c >>> +++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c >>> @@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int >>> budget) >>> ixgb_clean_rx_irq(adapter, &work_done, budget); >>>=20 >>> /* If budget not fully consumed, exit the polling mode */ >>> - if (work_done < budget) { >>> - napi_complete_done(napi, work_done); >>> + if (work_done < budget && >>> + napi_complete_done(napi, work_done)) { >>> if (!test_bit(__IXGB_DOWN, &adapter->flags)) >>> ixgb_irq_enable(adapter); >>> } >>>=20 >>=20 >>=20 >> ixgbe patch would be : >>=20 >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> index >> 604282f03d236e4358fc91e64d8ba00a9b36cb8c..80d00aecb6e3e3e950ce6309bfe >> 3639953dd73d9 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> @@ -3196,12 +3196,12 @@ int ixgbe_poll(struct napi_struct *napi, int >> budget) >> return budget; >>=20 >> /* all work done, exit the polling mode */ >> - napi_complete_done(napi, work_done); >> - if (adapter->rx_itr_setting & 1) >> - ixgbe_set_itr(q_vector); >> - if (!test_bit(__IXGBE_DOWN, &adapter->state)) >> - ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector- >>> v_idx)); >> - >> + if (napi_complete_done(napi, work_done)) { Shall we add likely() here?=20 >> + if (adapter->rx_itr_setting & 1) >> + ixgbe_set_itr(q_vector); >> + if (!test_bit(__IXGBE_DOWN, &adapter->state)) >> + ixgbe_irq_enable_queues(adapter, >> BIT_ULL(q_vector->v_idx)); >> + } >> return min(work_done, budget - 1); >> } >>=20 >=20 > Eric, after Song does some testing on these changes, will you be > submitting a formal patch? If so, make sure to send it to=20 > intel-wired-lan@lists.osuosl.org mailing list so I can pick up the fix. >=20 > By the way, thanks! I would submit the patch if Eric prefer not to. :) Thanks, Song=