| Message ID | alpine.OSX.2.00.1510291841480.68875@animac.local |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2015-10-29 at 18:41 -0700, Ani Sinha wrote: > > Signed-off-by: Ani Sinha <ani@arista.com> > --- > net/ipv4/ipmr.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > index 866ee89..48df3cc 100644 > --- a/net/ipv4/ipmr.c > +++ b/net/ipv4/ipmr.c > @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt, > > rtnl_unicast(skb, net, NETLINK_CB(skb).portid); > } else { > + preempt_disable(); > ip_mr_forward(net, mrt, skb, c, 0); > + preempt_enable(); > } > } > } I do not believe this fix is correct. Better replace the IP_INC_STATS_BH() by IP_INC_STATS() and IP_ADD_STATS_BH() by IP_ADD_STATS() -- 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
Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Signed-off-by: Ani Sinha <ani@arista.com> > > --- > > net/ipv4/ipmr.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > > index 866ee89..48df3cc 100644 > > --- a/net/ipv4/ipmr.c > > +++ b/net/ipv4/ipmr.c > > @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt, > > > > rtnl_unicast(skb, net, NETLINK_CB(skb).portid); > > } else { > > + preempt_disable(); > > ip_mr_forward(net, mrt, skb, c, 0); > > + preempt_enable(); > > } > > } > > } > > I do not believe this fix is correct. Yes, sorry. I should have suggested local_bh_disable instead. > Better replace the > IP_INC_STATS_BH() by IP_INC_STATS() > > and IP_ADD_STATS_BH() by IP_ADD_STATS() Hmm, whats the rationale for this? Note that IP_ADD_STATS_BH in question is unconditional (not in error path). It seems that its virtually always called from softirq except in the setsockopt case. Thanks Eric. -- 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 Fri, Oct 30, 2015, at 11:36, Florian Westphal wrote: > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > Signed-off-by: Ani Sinha <ani@arista.com> > > > --- > > > net/ipv4/ipmr.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > > > index 866ee89..48df3cc 100644 > > > --- a/net/ipv4/ipmr.c > > > +++ b/net/ipv4/ipmr.c > > > @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt, > > > > > > rtnl_unicast(skb, net, NETLINK_CB(skb).portid); > > > } else { > > > + preempt_disable(); > > > ip_mr_forward(net, mrt, skb, c, 0); > > > + preempt_enable(); > > > } > > > } > > > } > > > > I do not believe this fix is correct. > > Yes, sorry. I should have suggested local_bh_disable instead. > > > Better replace the > > IP_INC_STATS_BH() by IP_INC_STATS() > > > > and IP_ADD_STATS_BH() by IP_ADD_STATS() > > Hmm, whats the rationale for this? > > Note that IP_ADD_STATS_BH in question is unconditional (not in > error path). It seems that its virtually always called from softirq > except in the setsockopt case. The naming of the functions is bad if you compare them to e.g. spin_lock_bh. STATS_BH can only be used from bottom half and the normal ones (without _BH) can be called from everywhere. It is a common pattern in the kernel. Eric's proposal is correct. Bye, Hannes -- 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
Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > > > @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt, > > > > > > > > rtnl_unicast(skb, net, NETLINK_CB(skb).portid); > > > > } else { > > > > + preempt_disable(); > > > > ip_mr_forward(net, mrt, skb, c, 0); > > > > + preempt_enable(); > > > > } > > > > } > > > > } > > > > > > I do not believe this fix is correct. > > > > Yes, sorry. I should have suggested local_bh_disable instead. > > > > > Better replace the > > > IP_INC_STATS_BH() by IP_INC_STATS() > > > > > > and IP_ADD_STATS_BH() by IP_ADD_STATS() > > > > Hmm, whats the rationale for this? > > > > Note that IP_ADD_STATS_BH in question is unconditional (not in > > error path). It seems that its virtually always called from softirq > > except in the setsockopt case. > > The naming of the functions is bad if you compare them to e.g. > spin_lock_bh. > > STATS_BH can only be used from bottom half and the normal ones (without > _BH) can be called from everywhere. It is a common pattern in the > kernel. > > Eric's proposal is correct. Yes, its correct but it results in 4 additonal bh on/off calls for the common case, hence my question. Moving the one ip_mr_forward into bh-off keeps the bh-disable thing in the setsockopt path. -- 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 Fri, 2015-10-30 at 11:48 +0100, Florian Westphal wrote: > Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > > > > @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt, > > > > > > > > > > rtnl_unicast(skb, net, NETLINK_CB(skb).portid); > > > > > } else { > > > > > + preempt_disable(); > > > > > ip_mr_forward(net, mrt, skb, c, 0); > > > > > + preempt_enable(); > > > > > } > > > > > } > > > > > } > > > > > > > > I do not believe this fix is correct. > > > > > > Yes, sorry. I should have suggested local_bh_disable instead. > > > > > > > Better replace the > > > > IP_INC_STATS_BH() by IP_INC_STATS() > > > > > > > > and IP_ADD_STATS_BH() by IP_ADD_STATS() > > > > > > Hmm, whats the rationale for this? > > > > > > Note that IP_ADD_STATS_BH in question is unconditional (not in > > > error path). It seems that its virtually always called from softirq > > > except in the setsockopt case. > > > > The naming of the functions is bad if you compare them to e.g. > > spin_lock_bh. > > > > STATS_BH can only be used from bottom half and the normal ones (without > > _BH) can be called from everywhere. It is a common pattern in the > > kernel. > > > > Eric's proposal is correct. > > Yes, its correct but it results in 4 additonal bh on/off calls > for the common case, hence my question. > > Moving the one ip_mr_forward into bh-off keeps the bh-disable thing > in the setsockopt path. I have no idea how long is the ip_mr_forward(net, mrt, skb, c, 0) section, and if GFP_KERNEL allocations were attempted in this path. The proposed fix might add other regressions. I do not want to spend time auditing this code that nobody uses. While on x86, IP_INC_STATS() does not use additional bh on/off calls In general, we should disable interrupts (even if soft) for limited amount of times. -- 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 Fri, Oct 30, 2015 at 4:00 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2015-10-30 at 11:48 +0100, Florian Westphal wrote: >> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: >> > > > > @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt, >> > > > > >> > > > > rtnl_unicast(skb, net, NETLINK_CB(skb).portid); >> > > > > } else { >> > > > > + preempt_disable(); >> > > > > ip_mr_forward(net, mrt, skb, c, 0); >> > > > > + preempt_enable(); >> > > > > } >> > > > > } >> > > > > } >> > > > >> > > > I do not believe this fix is correct. >> > > >> > > Yes, sorry. I should have suggested local_bh_disable instead. >> > > >> > > > Better replace the >> > > > IP_INC_STATS_BH() by IP_INC_STATS() >> > > > >> > > > and IP_ADD_STATS_BH() by IP_ADD_STATS() >> > > >> > > Hmm, whats the rationale for this? >> > > >> > > Note that IP_ADD_STATS_BH in question is unconditional (not in >> > > error path). It seems that its virtually always called from softirq >> > > except in the setsockopt case. >> > >> > The naming of the functions is bad if you compare them to e.g. >> > spin_lock_bh. >> > >> > STATS_BH can only be used from bottom half and the normal ones (without >> > _BH) can be called from everywhere. It is a common pattern in the >> > kernel. >> > >> > Eric's proposal is correct. >> >> Yes, its correct but it results in 4 additonal bh on/off calls >> for the common case, hence my question. >> >> Moving the one ip_mr_forward into bh-off keeps the bh-disable thing >> in the setsockopt path. > > I have no idea how long is the ip_mr_forward(net, mrt, skb, c, 0) > section, and if GFP_KERNEL allocations were attempted in this path. > > The proposed fix might add other regressions. > > I do not want to spend time auditing this code that nobody uses. > > While on x86, IP_INC_STATS() does not use additional bh on/off calls > for 32 bit archs, it does in SNMP_ADD_STATS64_USER() > In general, we should disable interrupts (even if soft) for limited > amount of times. > > -- 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 Fri, 2015-10-30 at 10:47 -0700, Ani Sinha wrote:
> for 32 bit archs, it does in SNMP_ADD_STATS64_USER()
Sure. But x86 these days is 64bit, at 99 % maybe.
We do not make changes that looks 'maybe better' for i486 or i586
Just do the same that multiple similar patches did.
Example :
757efd32d5ce31f67193cc0e6a56e4dffcc42fb1
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 Fri, Oct 30, 2015 at 12:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2015-10-30 at 10:47 -0700, Ani Sinha wrote: > >> for 32 bit archs, it does in SNMP_ADD_STATS64_USER() > > Sure. But x86 these days is 64bit, at 99 % maybe. > > We do not make changes that looks 'maybe better' for i486 or i586 > > Just do the same that multiple similar patches did. > > Example : > > 757efd32d5ce31f67193cc0e6a56e4dffcc42fb1 OK thanks for pointing me to this. Seems we have a precedence for this I will go ahead and send a patch as per your suggestion. -- 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/ipv4/ipmr.c b/net/ipv4/ipmr.c index 866ee89..48df3cc 100644 --- a/net/ipv4/ipmr.c +++ b/net/ipv4/ipmr.c @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt, rtnl_unicast(skb, net, NETLINK_CB(skb).portid); } else { + preempt_disable(); ip_mr_forward(net, mrt, skb, c, 0); + preempt_enable(); } } }