Re: kernel BUG in ipmr_queue_xmit()
On Fri, Oct 30, 2015 at 12:12 PM, Eric Dumazet 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG in ipmr_queue_xmit()
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG in ipmr_queue_xmit()
On Fri, Oct 30, 2015 at 4:00 AM, Eric Dumazet wrote:
> On Fri, 2015-10-30 at 11:48 +0100, Florian Westphal wrote:
>> Hannes Frederic Sowa 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG in ipmr_queue_xmit()
On Fri, 2015-10-30 at 11:48 +0100, Florian Westphal wrote:
> Hannes Frederic Sowa 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG in ipmr_queue_xmit()
Hannes Frederic Sowa 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG in ipmr_queue_xmit()
On Fri, Oct 30, 2015, at 11:36, Florian Westphal wrote:
> Eric Dumazet wrote:
> > > Signed-off-by: Ani Sinha
> > > ---
> > > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG in ipmr_queue_xmit()
Eric Dumazet wrote:
> > Signed-off-by: Ani Sinha
> > ---
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG in ipmr_queue_xmit()
On Thu, 2015-10-29 at 18:41 -0700, Ani Sinha wrote:
>
> Signed-off-by: Ani Sinha
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
