[PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[Xiao Guangrong]

The net_dev of backlog napi is NULL, like below:

__get_cpu_var(softnet_data).backlog.dev == NULL

So, we should check it in napi tracepoint's probe function

Acked-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 net/core/drop_monitor.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 9d66fa9..d311202 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -182,7 +182,8 @@ static void trace_napi_poll_hit(struct napi_struct *napi)
 	/*
 	 * Ratelimit our check time to dm_hw_check_delta jiffies
 	 */
-	if (!time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
+	if (!napi->dev ||
+	    !time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
 		return;
 
 	rcu_read_lock();
-- 
1.6.1.2

Re: [PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[Eric Dumazet]

Xiao Guangrong a écrit :
> The net_dev of backlog napi is NULL, like below:
> 
> __get_cpu_var(softnet_data).backlog.dev == NULL
> 
> So, we should check it in napi tracepoint's probe function
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> ---
>  net/core/drop_monitor.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 9d66fa9..d311202 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -182,7 +182,8 @@ static void trace_napi_poll_hit(struct napi_struct *napi)
>  	/*
>  	 * Ratelimit our check time to dm_hw_check_delta jiffies
>  	 */
> -	if (!time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
> +	if (!napi->dev ||
> +	    !time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
>  		return;
>  
>  	rcu_read_lock();


This reminds me dev->last_rx is not anymore updated, unless special conditions
are met.

Test done in trace_napi_poll_hit() is probably not good, even with a non null napi->dev

Re: [PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[Neil Horman]

On Mon, Aug 31, 2009 at 08:31:51AM +0200, Eric Dumazet wrote:
> Xiao Guangrong a écrit :
> > The net_dev of backlog napi is NULL, like below:
> > 
> > __get_cpu_var(softnet_data).backlog.dev == NULL
> > 
> > So, we should check it in napi tracepoint's probe function
> > 
> > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > ---
> >  net/core/drop_monitor.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> > index 9d66fa9..d311202 100644
> > --- a/net/core/drop_monitor.c
> > +++ b/net/core/drop_monitor.c
> > @@ -182,7 +182,8 @@ static void trace_napi_poll_hit(struct napi_struct *napi)
> >  	/*
> >  	 * Ratelimit our check time to dm_hw_check_delta jiffies
> >  	 */
> > -	if (!time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
> > +	if (!napi->dev ||
> > +	    !time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
> >  		return;
> >  
> >  	rcu_read_lock();
> 
> 
> This reminds me dev->last_rx is not anymore updated, unless special conditions
> are met.
> 
I still see a large number of drivers that update dev->last_rx, although its
not all as I look through the list, so something definately seems amiss.

If its not going to be consistently updated, why are still carrying that field
in dev?  Are we just waiting on someone to do the janitorial work to remove it?
If so, I can, and I'll fix up the drop monitor in the process, to use a private
timestamp.

Neil

> Test done in trace_napi_poll_hit() is probably not good, even with a non null napi->dev
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 31 Aug 2009 07:12:46 -0400

> If its not going to be consistently updated, why are still carrying
> that field in dev?  Are we just waiting on someone to do the
> janitorial work to remove it?  If so, I can, and I'll fix up the
> drop monitor in the process, to use a private timestamp.

It's used only for bonding, so we only update it when a device
receives packet as part of a bond.

%99.9999 of people are not in that situation, and in that case this is
a very wasteful and expensive cacheline dirtying, so we elide it when
we can.

Re: [PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[Eric Dumazet]

Neil Horman a écrit :
> I still see a large number of drivers that update dev->last_rx, although its
> not all as I look through the list, so something definately seems amiss.

Some drivers still update dev->last_rx for their own needs, not a core
network concern.

But a cleanup is certainly possible on few other drivers, about a dozen
if I count correctly.

> 
> If its not going to be consistently updated, why are still carrying that field
> in dev?  Are we just waiting on someone to do the janitorial work to remove it?
> If so, I can, and I'll fix up the drop monitor in the process, to use a private
> timestamp.

We have to keep dev->last_rx for bonding use, so please use a private
timestamp for drop monitor.

Thanks

Re: [PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[Neil Horman]

On Mon, Aug 31, 2009 at 03:33:50PM +0200, Eric Dumazet wrote:
> Neil Horman a écrit :
> > I still see a large number of drivers that update dev->last_rx, although its
> > not all as I look through the list, so something definately seems amiss.
> 
> Some drivers still update dev->last_rx for their own needs, not a core
> network concern.
> 
> But a cleanup is certainly possible on few other drivers, about a dozen
> if I count correctly.
> 
> > 
> > If its not going to be consistently updated, why are still carrying that field
> > in dev?  Are we just waiting on someone to do the janitorial work to remove it?
> > If so, I can, and I'll fix up the drop monitor in the process, to use a private
> > timestamp.
> 
> We have to keep dev->last_rx for bonding use, so please use a private
> timestamp for drop monitor.
> 

Copy that, I'll submit a drop monitor patch for this sometime this week.  Thanks
for the heads up!
Neil

> Thanks
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH resend] drop_monitor: fix trace_napi_poll_hit()

[David Miller]

From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Date: Mon, 31 Aug 2009 14:10:43 +0800

> The net_dev of backlog napi is NULL, like below:
> 
> __get_cpu_var(softnet_data).backlog.dev == NULL
> 
> So, we should check it in napi tracepoint's probe function
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>

Applied to net-next-2.6, thanks.