MIME-Version: 1.0
References: <DM6PR12MB45165BFF3AB84602238FA595D89B9@DM6PR12MB4516.namprd12.prod.outlook.com>
In-Reply-To: <DM6PR12MB45165BFF3AB84602238FA595D89B9@DM6PR12MB4516.namprd12.prod.outlook.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Thu, 18 Nov 2021 08:14:22 -0800
Message-ID: <CAKgT0UfGvcGXAC5VBjXRpR5Y5uAPEPPCsYWjQR8RmW_1kw8TMQ@mail.gmail.com>
Subject: Re: [igb] netconsole triggers warning in netpoll_poll_dev
To:     Danielle Ratson <danieller@nvidia.com>
Cc:     Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Oleksandr Natalenko <oleksandr@natalenko.name>,
        Jakub Kicinski <kuba@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        intel-wired-lan <intel-wired-lan@lists.osuosl.org>,
        Netdev <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Nov 18, 2021 at 7:30 AM Danielle Ratson <danieller@nvidia.com> wrote:
>
> > On Thu, May 6, 2021 at 4:32 PM Jesse Brandeburg
> > <jesse.brandeburg@intel.com> wrote:
> > >
> > > Alexander Duyck wrote:
> > >
> > > > On Sun, Apr 25, 2021 at 11:47 PM Oleksandr Natalenko
> > > > <oleksandr@natalenko.name> wrote:
> > > > >
> > > > > Hello.
> > > > >
> > > > > On Fri, Apr 23, 2021 at 03:58:36PM -0700, Jakub Kicinski wrote:
> > > > > > On Fri, 23 Apr 2021 10:19:44 +0200 Oleksandr Natalenko wrote:
> > > > > > > On Wed, Apr 07, 2021 at 04:06:29PM -0700, Alexander Duyck wrote:
> > > > > > > > On Wed, Apr 7, 2021 at 11:07 AM Jakub Kicinski
> > <kuba@kernel.org> wrote:
> > > > > > > > > Sure, that's simplest. I wasn't sure something is supposed
> > > > > > > > > to prevent this condition or if it's okay to cover it up.
> > > > > > > >
> > > > > > > > I'm pretty sure it is okay to cover it up. In this case the
> > > > > > > > "budget - 1" is supposed to be the upper limit on what can
> > > > > > > > be reported. I think it was assuming an unsigned value anyway.
> > > > > > > >
> > > > > > > > Another alternative would be to default clean_complete to
> > !!budget.
> > > > > > > > Then if budget is 0 clean_complete would always return false.
> > > > > > >
> > > > > > > So, among all the variants, which one to try? Or there was a
> > > > > > > separate patch sent to address this?
> > > > > >
> > > > > > Alex's suggestion is probably best.
> > > > > >
> > > > > > I'm not aware of the fix being posted. Perhaps you could take
> > > > > > over and post the patch if Intel doesn't chime in?
> > > > >
> > > > > So, IIUC, Alex suggests this:
> > > > >
> > > > > ```
> > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> > > > > b/drivers/net/ethernet/intel/igb/igb_main.c
> > > > > index a45cd2b416c8..7503d5bf168a 100644
> > > > > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > > > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > > > > @@ -7981,7 +7981,7 @@ static int igb_poll(struct napi_struct *napi, int
> > budget)
> > > > >                                                      struct igb_q_vector,
> > > > >                                                      napi);
> > > > >         bool clean_complete = true;
> > > > > -       int work_done = 0;
> > > > > +       unsigned int work_done = 0;
> > > > >
> > > > >  #ifdef CONFIG_IGB_DCA
> > > > >         if (q_vector->adapter->flags & IGB_FLAG_DCA_ENABLED) @@
> > > > > -8008,7 +8008,7 @@ static int igb_poll(struct napi_struct *napi, int
> > budget)
> > > > >         if (likely(napi_complete_done(napi, work_done)))
> > > > >                 igb_ring_irq_enable(q_vector);
> > > > >
> > > > > -       return min(work_done, budget - 1);
> > > > > +       return min_t(unsigned int, work_done, budget - 1);
> > > > >  }
> > > > >
> > > > >  /**
> > > > > ```
> > > > >
> > > > > Am I right?
> > > > >
> > > > > Thanks.
> > > >
> > > > Actually a better way to go would be to probably just initialize
> > > > "clean_complete = !!budget". With that we don't have it messing with
> > > > the interrupt enables which would probably be a better behavior.
> > >
> > >
> > > Thanks guys for the suggestions here! Finally got some time for this,
> > > so here is the patch I'm going to queue shortly.
> > >
> > > From ffd24e90d688ee347ab051266bfc7fca00324a68 Mon Sep 17 00:00:00
> > 2001
> > > From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > > Date: Thu, 6 May 2021 14:41:11 -0700
> > > Subject: [PATCH net] igb: fix netpoll exit with traffic
> > > To: netdev,
> > >     Oleksandr Natalenko <oleksandr@natalenko.name>
> > > Cc: Jakub Kicinski <kuba@kernel.org>, LKML
> > > <linux-kernel@vger.kernel.org>, "Brandeburg, Jesse"
> > > <jesse.brandeburg@intel.com>, "Nguyen, Anthony L"
> > > <anthony.l.nguyen@intel.com>, "David S. Miller"
> > <davem@davemloft.net>,
> > > intel-wired-lan <intel-wired-lan@lists.osuosl.org>, Alexander Duyck
> > > <alexander.duyck@gmail.com>
> > >
> > > Oleksandr brought a bug report where netpoll causes trace messages in
> > > the log on igb.
> > >
> > > [22038.710800] ------------[ cut here ]------------ [22038.710801]
> > > igb_poll+0x0/0x1440 [igb] exceeded budget in poll [22038.710802]
> > > WARNING: CPU: 12 PID: 40362 at net/core/netpoll.c:155
> > > netpoll_poll_dev+0x18a/0x1a0
> > >
> > > After some discussion and debug from the list, it was deemed that the
> > > right thing to do is initialize the clean_complete variable to false
> > > when the "netpoll mode" of passing a zero budget is used.
> > >
> > > This logic should be sane and not risky because the only time budget
> > > should be zero on entry is netpoll.  Change includes a small refactor
> > > of local variable assignments to clean up the look.
> > >
> > > Fixes: 16eb8815c235 ("igb: Refactor clean_rx_irq to reduce overhead
> > > and improve performance")
> > > Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
> > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > > ---
> > >
> > > Compile tested ONLY, but functionally it should be exactly the same
> > > for all cases except when budget is zero on entry, which will
> > > hopefully fix the bug.
> > > ---
> > >  drivers/net/ethernet/intel/igb/igb_main.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> > > b/drivers/net/ethernet/intel/igb/igb_main.c
> > > index 0cd37ad81b4e..b0a9bed14071 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > > @@ -7991,12 +7991,16 @@ static void igb_ring_irq_enable(struct
> > igb_q_vector *q_vector)
> > >   **/
> > >  static int igb_poll(struct napi_struct *napi, int budget)  {
> > > -       struct igb_q_vector *q_vector = container_of(napi,
> > > -                                                    struct igb_q_vector,
> > > -                                                    napi);
> > > -       bool clean_complete = true;
> > > +       struct igb_q_vector *q_vector;
> > > +       bool clean_complete;
> > >         int work_done = 0;
> > >
> > > +       /* if budget is zero, we have a special case for netconsole, so
> > > +        * make sure to set clean_complete to false in that case.
> > > +        */
> > > +       clean_complete = !!budget;
> > > +
> > > +       q_vector = container_of(napi, struct igb_q_vector, napi);
> > >  #ifdef CONFIG_IGB_DCA
> > >         if (q_vector->adapter->flags & IGB_FLAG_DCA_ENABLED)
> > >                 igb_update_dca(q_vector);
> >
> > I'm not a big fan of moving the q_vector init as a part of this patch since it
> > just means more backport work.
> >
> > That said the change itself should be harmless so I am good with it either
> > way.
> >
> > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
>
> Hi,
>
> I have lately added the netconsole module, and since then we see the same warning constantly in the logs.
> I have tried to apply Jesse's patch but it didn't seem to solve the issue.
>
> Did anyone managed to solve the issue and can share with us?
>
> Thanks,
> Danielle

The one issue I can see is that it basically leaves the igb_poll call
stuck in polling mode.

The easiest fix for all of this in the in-kernel driver is to just get
rid of the "min" at the end and instead just "return work_done;". The
extra complication is only needed if you were to be polling multiple
queues and that isn't the case here so we should simplify it and get
rid of the buggy "budget - 1" return value.