| Message ID | 4A9B6963.5090207@cn.fujitsu.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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
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 > -- 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
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. -- 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
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 -- 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
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 > -- 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
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. -- 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
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();