[PATCH 6/7] : tehuti Fix possible causing oops of net_rx_action

[PATCH 6/7] : tehuti Fix possible causing oops of net_rx_action

[Joonwoo Park]

[NETDEV]: tehuti Fix possible causing oops of net_rx_action

Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
---
 drivers/net/tehuti.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tehuti.c b/drivers/net/tehuti.c
index 21230c9..955e749 100644
--- a/drivers/net/tehuti.c
+++ b/drivers/net/tehuti.c
@@ -305,6 +305,8 @@ static int bdx_poll(struct napi_struct *napi, int budget)
 
 		netif_rx_complete(dev, napi);
 		bdx_enable_interrupts(priv);
+		if (unlikely(work_done == napi->weight))
+			return work_done - 1;
 	}
 	return work_done;
 }
---

Re: [PATCH 6/7] : tehuti Fix possible causing oops of net_rx_action

[Stephen Hemminger]

On Wed, 12 Dec 2007 13:01:27 +0900
"Joonwoo Park" <joonwpark81@gmail.com> wrote:

> [NETDEV]: tehuti Fix possible causing oops of net_rx_action
> 
> Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> ---
>  drivers/net/tehuti.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/tehuti.c b/drivers/net/tehuti.c
> index 21230c9..955e749 100644
> --- a/drivers/net/tehuti.c
> +++ b/drivers/net/tehuti.c
> @@ -305,6 +305,8 @@ static int bdx_poll(struct napi_struct *napi, int budget)
>  
>  		netif_rx_complete(dev, napi);
>  		bdx_enable_interrupts(priv);
> +		if (unlikely(work_done == napi->weight))
> +			return work_done - 1;
>  	}
>  	return work_done;
>  }

A better fix would be not going over budget in the first place.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[RFC] net: napi fix

[Stephen Hemminger]

Isn't this a better fix for all drivers, rather than peppering every
driver with the special case. This is how the logic worked up until
2.6.24.


--- a/net/core/dev.c	2007-12-11 12:16:20.000000000 -0800
+++ b/net/core/dev.c	2007-12-11 21:43:39.000000000 -0800
@@ -2184,7 +2184,7 @@ static void net_rx_action(struct softirq
 
 		have = netpoll_poll_lock(n);
 
-		weight = n->weight;
+		weight = min(n->weight, budget);
 
 		/* This NAPI_STATE_SCHED test is for avoiding a race
 		 * with netpoll's poll_napi().  Only the entity which

Re: [RFC] net: napi fix

[Joonwoo Park]

2007/12/12, Stephen Hemminger <shemminger@linux-foundation.org>:
> Isn't this a better fix for all drivers, rather than peppering every
> driver with the special case. This is how the logic worked up until
> 2.6.24.
>
>
> --- a/net/core/dev.c    2007-12-11 12:16:20.000000000 -0800
> +++ b/net/core/dev.c    2007-12-11 21:43:39.000000000 -0800
> @@ -2184,7 +2184,7 @@ static void net_rx_action(struct softirq
>
>                have = netpoll_poll_lock(n);
>
> -               weight = n->weight;
> +               weight = min(n->weight, budget);
>
>                /* This NAPI_STATE_SCHED test is for avoiding a race
>                 * with netpoll's poll_napi().  Only the entity which
>

Stephen,
Could you explain how it fix the problem?
IMHO I think your patch cannot solve the problem.
The drivers can call netif_rx_complete and net_rx_action can do
list_move_tail also.
Am I missing something?

Thanks
Joonwoo

Re: [RFC] net: napi fix

[David Miller]

From: "Joonwoo Park" <joonwpark81@gmail.com>
Date: Wed, 12 Dec 2007 15:05:26 +0900

> Could you explain how it fix the problem?
> IMHO I think your patch cannot solve the problem.
> The drivers can call netif_rx_complete and net_rx_action can do
> list_move_tail also.

Stephen is confused about what the bug is in these drivers,
that's all.

Re: [RFC] net: napi fix

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Tue, 11 Dec 2007 21:46:34 -0800

> Isn't this a better fix for all drivers, rather than peppering every
> driver with the special case. This is how the logic worked up until
> 2.6.24.

Stephen this is not the problem.

The problem is that the driver is doing a NAPI completion and
re-enabling chip interrupts with work_done == weight, and that is
illegal.

Re: [PATCH 6/7] : tehuti Fix possible causing oops of net_rx_action

[Joonwoo Park]

2007/12/12, Stephen Hemminger <shemminger@linux-foundation.org>:
> On Wed, 12 Dec 2007 13:01:27 +0900
> "Joonwoo Park" <joonwpark81@gmail.com> wrote:
>
> > [NETDEV]: tehuti Fix possible causing oops of net_rx_action
> >
> > Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> > ---
> >  drivers/net/tehuti.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/tehuti.c b/drivers/net/tehuti.c
> > index 21230c9..955e749 100644
> > --- a/drivers/net/tehuti.c
> > +++ b/drivers/net/tehuti.c
> > @@ -305,6 +305,8 @@ static int bdx_poll(struct napi_struct *napi, int budget)
> >
> >               netif_rx_complete(dev, napi);
> >               bdx_enable_interrupts(priv);
> > +             if (unlikely(work_done == napi->weight))
> > +                     return work_done - 1;
> >       }
> >       return work_done;
> >  }
>
> A better fix would be not going over budget in the first place.
>
> --
> Stephen Hemminger <shemminger@linux-foundation.org>
>

Stephen,
This is code of bd_poll().
Do you mean remove napi_stop stuff?

static int bdx_poll(struct napi_struct *napi, int budget)
{
...
        work_done = bdx_rx_receive(priv, &priv->rxd_fifo0, budget);
        if ((work_done < budget) ||
            (priv->napi_stop++ >= 30)) {
                DBG("rx poll is done. backing to isr-driven\n");

                /* from time to time we exit to let NAPI layer release
                 * device lock and allow waiting tasks (eg rmmod) to advance) */
                priv->napi_stop = 0;

                netif_rx_complete(dev, napi);
                bdx_enable_interrupts(priv);
                if (unlikely(work_done == napi->weight))
                        return work_done - 1;
        }
        return work_done;
}

Thanks,
Joonwoo

Re: [PATCH 6/7] : tehuti Fix possible causing oops of net_rx_action

[Stephen Hemminger]

On Wed, 12 Dec 2007 14:48:27 +0900
"Joonwoo Park" <joonwpark81@gmail.com> wrote:

> 2007/12/12, Stephen Hemminger <shemminger@linux-foundation.org>:
> > On Wed, 12 Dec 2007 13:01:27 +0900
> > "Joonwoo Park" <joonwpark81@gmail.com> wrote:
> >
> > > [NETDEV]: tehuti Fix possible causing oops of net_rx_action
> > >
> > > Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> > > ---
> > >  drivers/net/tehuti.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/net/tehuti.c b/drivers/net/tehuti.c
> > > index 21230c9..955e749 100644
> > > --- a/drivers/net/tehuti.c
> > > +++ b/drivers/net/tehuti.c
> > > @@ -305,6 +305,8 @@ static int bdx_poll(struct napi_struct *napi, int budget)
> > >
> > >               netif_rx_complete(dev, napi);
> > >               bdx_enable_interrupts(priv);
> > > +             if (unlikely(work_done == napi->weight))
> > > +                     return work_done - 1;
> > >       }
> > >       return work_done;
> > >  }
> >
> > A better fix would be not going over budget in the first place.
> >
> > --
> > Stephen Hemminger <shemminger@linux-foundation.org>
> >
> 
> Stephen,
> This is code of bd_poll().
> Do you mean remove napi_stop stuff?
> 
> static int bdx_poll(struct napi_struct *napi, int budget)
> {
> ...
>         work_done = bdx_rx_receive(priv, &priv->rxd_fifo0, budget);
>         if ((work_done < budget) ||
>             (priv->napi_stop++ >= 30)) {

Yes remove the napi_stop stuff, because current NAPI expects device
to be constrained only by budget. If you need to stop sooner, just
set napi weight to be smaller.

>                 DBG("rx poll is done. backing to isr-driven\n");
> 
>                 /* from time to time we exit to let NAPI layer release
>                  * device lock and allow waiting tasks (eg rmmod) to advance) */
>                 priv->napi_stop = 0;
> 
>                 netif_rx_complete(dev, napi);
>                 bdx_enable_interrupts(priv);

With my posted fix to rx_action the following two lines would not be needed.
>                 if (unlikely(work_done == napi->weight))
>                         return work_done - 1;
>         }
>         return work_done;
> }
> 
> Thanks,
> Joonwoo


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 6/7] [NETDEV]: tehuti Fix possible causing oops of net_rx_action

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Tue, 11 Dec 2007 21:39:39 -0800

> On Wed, 12 Dec 2007 13:01:27 +0900
> "Joonwoo Park" <joonwpark81@gmail.com> wrote:
> 
> > [NETDEV]: tehuti Fix possible causing oops of net_rx_action
> > 
> > Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> > ---
> >  drivers/net/tehuti.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/tehuti.c b/drivers/net/tehuti.c
> > index 21230c9..955e749 100644
> > --- a/drivers/net/tehuti.c
> > +++ b/drivers/net/tehuti.c
> > @@ -305,6 +305,8 @@ static int bdx_poll(struct napi_struct *napi, int budget)
> >  
> >  		netif_rx_complete(dev, napi);
> >  		bdx_enable_interrupts(priv);
> > +		if (unlikely(work_done == napi->weight))
> > +			return work_done - 1;
> >  	}
> >  	return work_done;
> >  }
> 
> A better fix would be not going over budget in the first place.

That's not the problem.

They are not going over the budget, rather, they are hitting
the budget yet doing netif_rx_complete() as well which is
illegal.

Unless you strictly process less than "weight" packets, you must
not netif_rx_complete() and re-enable chip interrupts.

I can't believe people are trying to fix this bug like this.

Re: [PATCH 6/7] [NETDEV]: tehuti Fix possible causing oops of net_rx_action

[Stephen Hemminger]

On Wed, 12 Dec 2007 07:20:34 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Tue, 11 Dec 2007 21:39:39 -0800
> 
> > On Wed, 12 Dec 2007 13:01:27 +0900
> > "Joonwoo Park" <joonwpark81@gmail.com> wrote:
> > 
> > > [NETDEV]: tehuti Fix possible causing oops of net_rx_action
> > > 
> > > Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> > > ---
> > >  drivers/net/tehuti.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/net/tehuti.c b/drivers/net/tehuti.c
> > > index 21230c9..955e749 100644
> > > --- a/drivers/net/tehuti.c
> > > +++ b/drivers/net/tehuti.c
> > > @@ -305,6 +305,8 @@ static int bdx_poll(struct napi_struct *napi, int budget)
> > >  
> > >  		netif_rx_complete(dev, napi);
> > >  		bdx_enable_interrupts(priv);
> > > +		if (unlikely(work_done == napi->weight))
> > > +			return work_done - 1;
> > >  	}
> > >  	return work_done;
> > >  }
> > 
> > A better fix would be not going over budget in the first place.
> 
> That's not the problem.
> 
> They are not going over the budget, rather, they are hitting
> the budget yet doing netif_rx_complete() as well which is
> illegal.
> 
> Unless you strictly process less than "weight" packets, you must
> not netif_rx_complete() and re-enable chip interrupts.
> 
> I can't believe people are trying to fix this bug like this.

Sorry, I was looking at a different possible problem. The issue
is that if netdev_budget was set smaller (say 128) but device
weight was set larger (say 256). The new code would still allow
the device to do a full swipe (256) packets rather than only
128 as in earlier NAPI. I guess it is an okay behaviour change, because
we don't really guarantee that case.

The problem with the tehuti driver is the logic around priv->napi_stop.
That whole early stop concept should be removed since it just
duplicates the logic of netdev->weight but breaks the assumptions
in the calling netif_rx_action.



-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 6/7] : tehuti Fix possible causing oops of net_rx_action

[David Miller]

From: "Joonwoo Park" <joonwpark81@gmail.com>
Date: Wed, 12 Dec 2007 13:01:27 +0900

> @@ -305,6 +305,8 @@ static int bdx_poll(struct napi_struct *napi, int budget)
>  
>  		netif_rx_complete(dev, napi);
>  		bdx_enable_interrupts(priv);
> +		if (unlikely(work_done == napi->weight))
> +			return work_done - 1;
>  	}
>  	return work_done;
>  }

Any time your trying to make a caller "happy" by adjusting
a return value forcefully, it's a hack.

And I stated this in another reply about this issue.

Please do not fix the problem this way.

The correct way to fix this is, if we did process a full
"weight" or work, we should not netif_rx_complete() and
we should not re-enable chip interrupts.

Instead we should return the true "work_done" value and
allow the caller to thus poll us one more time.

[RFT] tehuti: napi fix

[Stephen Hemminger]

This should fix the tehuti napi fence post problems by getting
rid of priv->napi_stop, and setting weight to 32 (like other 10G).

Also, used the wierd entry/exit macro's like rest of driver.

--- a/drivers/net/tehuti.c	2007-11-13 22:19:14.000000000 -0800
+++ b/drivers/net/tehuti.c	2007-12-12 13:37:01.000000000 -0800
@@ -295,18 +295,12 @@ static int bdx_poll(struct napi_struct *
 	ENTER;
 	bdx_tx_cleanup(priv);
 	work_done = bdx_rx_receive(priv, &priv->rxd_fifo0, budget);
-	if ((work_done < budget) ||
-	    (priv->napi_stop++ >= 30)) {
+	if (work_done < budget) {
 		DBG("rx poll is done. backing to isr-driven\n");
-
-		/* from time to time we exit to let NAPI layer release
-		 * device lock and allow waiting tasks (eg rmmod) to advance) */
-		priv->napi_stop = 0;
-
 		netif_rx_complete(dev, napi);
 		bdx_enable_interrupts(priv);
 	}
-	return work_done;
+	RET(work_done);
 }
 
 /* bdx_fw_load - loads firmware to NIC
@@ -2022,7 +2016,7 @@ bdx_probe(struct pci_dev *pdev, const st
 		priv->nic = nic;
 		priv->msg_enable = BDX_DEF_MSG_ENABLE;
 
-		netif_napi_add(ndev, &priv->napi, bdx_poll, 64);
+		netif_napi_add(ndev, &priv->napi, bdx_poll, 32);
 
 		if ((readl(nic->regs + FPGA_VER) & 0xFFF) == 308) {
 			DBG("HW statistics not supported\n");
--- a/drivers/net/tehuti.h	2007-10-16 16:48:17.000000000 -0700
+++ b/drivers/net/tehuti.h	2007-12-12 13:25:25.000000000 -0800
@@ -257,7 +257,6 @@ struct bdx_priv {
 	struct rxd_fifo rxd_fifo0;
 	struct rxf_fifo rxf_fifo0;
 	struct rxdb *rxdb;	/* rx dbs to store skb pointers */
-	int napi_stop;
 	struct vlan_group *vlgrp;
 
 	/* Tx FIFOs: 1 for data desc, 1 for empty (acks) desc */

Re: [RFT] tehuti: napi fix

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 12 Dec 2007 13:58:52 -0800

> This should fix the tehuti napi fence post problems by getting
> rid of priv->napi_stop, and setting weight to 32 (like other 10G).
> 
> Also, used the wierd entry/exit macro's like rest of driver.

It fixes the fench-post problem, but like the comments you
removed explain:

> -		/* from time to time we exit to let NAPI layer release
> -		 * device lock and allow waiting tasks (eg rmmod) to advance) */
> -		priv->napi_stop = 0;
> -

We now hang on rmmod during constant packet load.

This change just trades one bug for another, we have to
get the device close issue sorted out before we can go
around removing these things.

Re: [RFT] tehuti: napi fix

[Stephen Hemminger]

On Sun, 16 Dec 2007 13:38:33 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Wed, 12 Dec 2007 13:58:52 -0800
> 
> > This should fix the tehuti napi fence post problems by getting
> > rid of priv->napi_stop, and setting weight to 32 (like other 10G).
> > 
> > Also, used the wierd entry/exit macro's like rest of driver.
> 
> It fixes the fench-post problem, but like the comments you
> removed explain:
> 
> > -		/* from time to time we exit to let NAPI layer release
> > -		 * device lock and allow waiting tasks (eg rmmod) to advance) */
> > -		priv->napi_stop = 0;
> > -
> 
> We now hang on rmmod during constant packet load.
> 
> This change just trades one bug for another, we have to
> get the device close issue sorted out before we can go
> around removing these things.

Well the napi_stop had the same effect as having a smaller weight
value, so my patch just shrunk the weight. That causes the device
to exit NAPI (and should solve the rmmod problem).


-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

Re: [PATCH 6/7] : tehuti Fix possible causing oops of net_rx_action

[Joonwoo Park]

2007/12/13, David Miller <davem@davemloft.net>:
> From: "Joonwoo Park" <joonwpark81@gmail.com>
> Date: Wed, 12 Dec 2007 13:01:27 +0900
>
>
> Any time your trying to make a caller "happy" by adjusting
> a return value forcefully, it's a hack.
>
> And I stated this in another reply about this issue.
>
> Please do not fix the problem this way.
>
> The correct way to fix this is, if we did process a full
> "weight" or work, we should not netif_rx_complete() and
> we should not re-enable chip interrupts.
>
> Instead we should return the true "work_done" value and
> allow the caller to thus poll us one more time.
>

Thanks so much for your advice.
I agree, returning work_done itself exactly.
I will rework for these drivers.

Thanks.
Joonwoo