* [PATCH net] sctp: use gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic when sctp_assoc_set_id
@ 2016-03-05 15:59 Xin Long
2016-03-05 16:42 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Xin Long @ 2016-03-05 15:59 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: Marcelo Ricardo Leitner, Vlad Yasevich, daniel, davem
The following call trace appears because of idr_alloc_cyclic(..., GFP_NOWAIT),
that is a stress test, and the reason should be a heavy use. it will cause
sctp_process_init return 0, and make connection init fail.
All the allocations of idr_alloc_cyclic should respect gfp flag.
So we can fix it by using gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic.
[ 2569.797532] Call Trace:
[ 2569.809721] [<ffffffff8163ed74>] dump_stack+0x19/0x1b
[ 2569.837424] [<ffffffff81171460>] warn_alloc_failed+0x110/0x180
[ 2569.867013] [<ffffffff81175be8>] __alloc_pages_nodemask+0x9a8/0xb90
[ 2569.900039] [<ffffffff811b68d9>] alloc_pages_current+0xa9/0x170
[ 2569.930100] [<ffffffff811c19dc>] new_slab+0x2ec/0x300
[ 2569.957505] [<ffffffff8163bcdf>] __slab_alloc+0x315/0x48f
[ 2569.985168] [<ffffffff812f6bd9>] ? idr_layer_alloc+0x89/0x90
[ 2570.014344] [<ffffffff812fedae>] ? memzero_explicit+0xe/0x10
[ 2570.044116] [<ffffffff811c3fa3>] kmem_cache_alloc+0x193/0x1d0
[ 2570.073283] [<ffffffff812f6bd9>] idr_layer_alloc+0x89/0x90
[ 2570.102684] [<ffffffff812f72dc>] idr_get_empty_slot+0x24c/0x3c0
[ 2570.132634] [<ffffffff812f75ac>] idr_alloc+0x5c/0x100
[ 2570.159835] [<ffffffffa08af6c0>] ? sctp_hash_transport+0x110/0x290 [sctp]
[ 2570.194974] [<ffffffff812f767b>] idr_alloc_cyclic+0x2b/0x60
[ 2570.223695] [<ffffffffa089a546>] sctp_assoc_set_id+0x46/0xd0 [sctp]
[ 2570.256245] [<ffffffffa089f321>] sctp_process_init+0x921/0x940 [sctp]
[ 2570.288574] [<ffffffffa08ae030>] ? sctp_csum_combine+0x10/0x10 [sctp]
[ 2570.321758] [<ffffffffa0896788>] sctp_cmd_interpreter.isra.25+0xe18/0x1330 [sctp]
[ 2570.459745] [<ffffffffa089543f>] sctp_do_sm+0xaf/0x1b0 [sctp]
[ 2570.489725] [<ffffffffa0899655>] sctp_assoc_bh_rcv+0xd5/0x140 [sctp]
[ 2570.522628] [<ffffffffa08a137c>] sctp_inq_push+0x4c/0x70 [sctp]
[ 2570.553812] [<ffffffffa08af160>] sctp_backlog_rcv+0x40/0x130 [sctp]
[ 2570.585534] [<ffffffff8151cbe1>] release_sock+0xa1/0x170
[ 2570.613632] [<ffffffffa08a8c1a>] __sctp_connect+0x41a/0x550 [sctp]
[ 2570.644998] [<ffffffff810a84e0>] ? wake_up_atomic_t+0x30/0x30
[ 2570.675601] [<ffffffff816463f2>] ? _raw_spin_lock_bh+0x12/0x50
[ 2570.705185] [<ffffffffa08a8e6d>] sctp_connect+0x4d/0x70 [sctp]
[ 2570.736234] [<ffffffff815a82ce>] inet_dgram_connect+0x2e/0x80
[ 2570.765514] [<ffffffff81518387>] SYSC_connect+0xe7/0x120
[ 2570.793691] [<ffffffff81515450>] ? sock_alloc_file+0xa0/0x140
[ 2570.822558] [<ffffffff811ffad7>] ? __fd_install+0x47/0x60
[ 2570.851732] [<ffffffff8151943e>] SyS_connect+0xe/0x10
[ 2570.877433] [<ffffffff8164f1c9>] system_call_fastpath+0x16/0x1b
[ 2570.908682] SLUB: Unable to allocate memory on node -1 (gfp=0x8000)
[ 2570.939959] cache: idr_layer_cache, object size: 2112, buffer size: 2112, default order: 3, min order: 0
[ 2570.989683] node 0: slabs: 839, objs: 11829, free: 0
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/associola.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 2bf8ec9..481187a 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1606,7 +1606,7 @@ int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
idr_preload(gfp);
spin_lock_bh(&sctp_assocs_id_lock);
/* 0 is not a valid assoc_id, must be >= 1 */
- ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0, GFP_NOWAIT);
+ ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0, gfp);
spin_unlock_bh(&sctp_assocs_id_lock);
if (preload)
idr_preload_end();
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: use gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic when sctp_assoc_set_id
2016-03-05 15:59 [PATCH net] sctp: use gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic when sctp_assoc_set_id Xin Long
@ 2016-03-05 16:42 ` Eric Dumazet
2016-03-06 5:52 ` Xin Long
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2016-03-05 16:42 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Vlad Yasevich,
daniel, davem
On sam., 2016-03-05 at 23:59 +0800, Xin Long wrote:
> The following call trace appears because of idr_alloc_cyclic(..., GFP_NOWAIT),
> that is a stress test, and the reason should be a heavy use. it will cause
> sctp_process_init return 0, and make connection init fail.
>
> All the allocations of idr_alloc_cyclic should respect gfp flag.
> So we can fix it by using gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic.
>
> [ 2569.797532] Call Trace:
> [ 2569.809721] [<ffffffff8163ed74>] dump_stack+0x19/0x1b
> [ 2569.837424] [<ffffffff81171460>] warn_alloc_failed+0x110/0x180
> [ 2569.867013] [<ffffffff81175be8>] __alloc_pages_nodemask+0x9a8/0xb90
> [ 2569.900039] [<ffffffff811b68d9>] alloc_pages_current+0xa9/0x170
> [ 2569.930100] [<ffffffff811c19dc>] new_slab+0x2ec/0x300
> [ 2569.957505] [<ffffffff8163bcdf>] __slab_alloc+0x315/0x48f
> [ 2569.985168] [<ffffffff812f6bd9>] ? idr_layer_alloc+0x89/0x90
> [ 2570.014344] [<ffffffff812fedae>] ? memzero_explicit+0xe/0x10
> [ 2570.044116] [<ffffffff811c3fa3>] kmem_cache_alloc+0x193/0x1d0
> [ 2570.073283] [<ffffffff812f6bd9>] idr_layer_alloc+0x89/0x90
> [ 2570.102684] [<ffffffff812f72dc>] idr_get_empty_slot+0x24c/0x3c0
> [ 2570.132634] [<ffffffff812f75ac>] idr_alloc+0x5c/0x100
> [ 2570.159835] [<ffffffffa08af6c0>] ? sctp_hash_transport+0x110/0x290 [sctp]
> [ 2570.194974] [<ffffffff812f767b>] idr_alloc_cyclic+0x2b/0x60
> [ 2570.223695] [<ffffffffa089a546>] sctp_assoc_set_id+0x46/0xd0 [sctp]
> [ 2570.256245] [<ffffffffa089f321>] sctp_process_init+0x921/0x940 [sctp]
> [ 2570.288574] [<ffffffffa08ae030>] ? sctp_csum_combine+0x10/0x10 [sctp]
> [ 2570.321758] [<ffffffffa0896788>] sctp_cmd_interpreter.isra.25+0xe18/0x1330 [sctp]
> [ 2570.459745] [<ffffffffa089543f>] sctp_do_sm+0xaf/0x1b0 [sctp]
> [ 2570.489725] [<ffffffffa0899655>] sctp_assoc_bh_rcv+0xd5/0x140 [sctp]
> [ 2570.522628] [<ffffffffa08a137c>] sctp_inq_push+0x4c/0x70 [sctp]
> [ 2570.553812] [<ffffffffa08af160>] sctp_backlog_rcv+0x40/0x130 [sctp]
> [ 2570.585534] [<ffffffff8151cbe1>] release_sock+0xa1/0x170
> [ 2570.613632] [<ffffffffa08a8c1a>] __sctp_connect+0x41a/0x550 [sctp]
> [ 2570.644998] [<ffffffff810a84e0>] ? wake_up_atomic_t+0x30/0x30
> [ 2570.675601] [<ffffffff816463f2>] ? _raw_spin_lock_bh+0x12/0x50
> [ 2570.705185] [<ffffffffa08a8e6d>] sctp_connect+0x4d/0x70 [sctp]
> [ 2570.736234] [<ffffffff815a82ce>] inet_dgram_connect+0x2e/0x80
> [ 2570.765514] [<ffffffff81518387>] SYSC_connect+0xe7/0x120
> [ 2570.793691] [<ffffffff81515450>] ? sock_alloc_file+0xa0/0x140
> [ 2570.822558] [<ffffffff811ffad7>] ? __fd_install+0x47/0x60
> [ 2570.851732] [<ffffffff8151943e>] SyS_connect+0xe/0x10
> [ 2570.877433] [<ffffffff8164f1c9>] system_call_fastpath+0x16/0x1b
> [ 2570.908682] SLUB: Unable to allocate memory on node -1 (gfp=0x8000)
> [ 2570.939959] cache: idr_layer_cache, object size: 2112, buffer size: 2112, default order: 3, min order: 0
> [ 2570.989683] node 0: slabs: 839, objs: 11829, free: 0
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/associola.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 2bf8ec9..481187a 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1606,7 +1606,7 @@ int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
> idr_preload(gfp);
> spin_lock_bh(&sctp_assocs_id_lock);
> /* 0 is not a valid assoc_id, must be >= 1 */
> - ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0, GFP_NOWAIT);
> + ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0, gfp);
> spin_unlock_bh(&sctp_assocs_id_lock);
> if (preload)
> idr_preload_end();
Are you sure idr_alloc(... GFP_KERNEL) makes sense inside spin_lock_bh()
section ?
idr_alloc() has :
might_sleep_if(gfpflags_allow_blocking(gfp_mask));
A debug kernel (CONFIG_DEBUG_ATOMIC_SLEEP=y) should probably complain at
this point ?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: use gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic when sctp_assoc_set_id
2016-03-05 16:42 ` Eric Dumazet
@ 2016-03-06 5:52 ` Xin Long
2016-03-06 23:21 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Xin Long @ 2016-03-06 5:52 UTC (permalink / raw)
To: Eric Dumazet
Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Vlad Yasevich,
daniel, davem
On Sun, Mar 6, 2016 at 12:42 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On sam., 2016-03-05 at 23:59 +0800, Xin Long wrote:
>
> Are you sure idr_alloc(... GFP_KERNEL) makes sense inside spin_lock_bh()
> section ?
>
> idr_alloc() has :
>
> might_sleep_if(gfpflags_allow_blocking(gfp_mask));
>
> A debug kernel (CONFIG_DEBUG_ATOMIC_SLEEP=y) should probably complain at
> this point ?
>
OK, I got you, does it make sense to you if I just change GFP_NOWAIT
to GFP_ATOMIC
when sctp_assoc_set_id call idr_alloc_cyclic()? which also can avoid
this call trace in my
test.
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: use gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic when sctp_assoc_set_id
2016-03-06 5:52 ` Xin Long
@ 2016-03-06 23:21 ` Eric Dumazet
2016-03-08 2:54 ` Xin Long
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2016-03-06 23:21 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Vlad Yasevich,
daniel, davem
On dim., 2016-03-06 at 13:52 +0800, Xin Long wrote:
> On Sun, Mar 6, 2016 at 12:42 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On sam., 2016-03-05 at 23:59 +0800, Xin Long wrote:
> >
> > Are you sure idr_alloc(... GFP_KERNEL) makes sense inside spin_lock_bh()
> > section ?
> >
> > idr_alloc() has :
> >
> > might_sleep_if(gfpflags_allow_blocking(gfp_mask));
> >
> > A debug kernel (CONFIG_DEBUG_ATOMIC_SLEEP=y) should probably complain at
> > this point ?
> >
> OK, I got you, does it make sense to you if I just change GFP_NOWAIT
> to GFP_ATOMIC
> when sctp_assoc_set_id call idr_alloc_cyclic()? which also can avoid
> this call trace in my
> test.
>
> >
> >
What is the problem of being not able to allocate memory at this point ?
If really it bothers you (although we have thousands of other places it
can happen), maybe add __GFP_NOWARN
But whatever flag we use here, idr_alloc() _can_ fail.
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 2bf8ec92dde4..2ae3874e3696 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1606,7 +1606,8 @@ int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
idr_preload(gfp);
spin_lock_bh(&sctp_assocs_id_lock);
/* 0 is not a valid assoc_id, must be >= 1 */
- ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0, GFP_NOWAIT);
+ ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0,
+ (gfp & ~__GFP_DIRECT_RECLAIM) | __GFP_NOWARN);
spin_unlock_bh(&sctp_assocs_id_lock);
if (preload)
idr_preload_end();
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: use gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic when sctp_assoc_set_id
2016-03-06 23:21 ` Eric Dumazet
@ 2016-03-08 2:54 ` Xin Long
0 siblings, 0 replies; 5+ messages in thread
From: Xin Long @ 2016-03-08 2:54 UTC (permalink / raw)
To: Eric Dumazet
Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Vlad Yasevich,
daniel, davem
On Mon, Mar 7, 2016 at 7:21 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> What is the problem of being not able to allocate memory at this point ?
>
Now I think about it again, this patch cannot work, because of:
__sctp_connect()
sctp_assoc_set_id(asoc, GFP_KERNEL)
thanks, Eric
> If really it bothers you (although we have thousands of other places it
> can happen), maybe add __GFP_NOWARN
>
> But whatever flag we use here, idr_alloc() _can_ fail.
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 2bf8ec92dde4..2ae3874e3696 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1606,7 +1606,8 @@ int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
> idr_preload(gfp);
> spin_lock_bh(&sctp_assocs_id_lock);
> /* 0 is not a valid assoc_id, must be >= 1 */
> - ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0, GFP_NOWAIT);
> + ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0,
> + (gfp & ~__GFP_DIRECT_RECLAIM) | __GFP_NOWARN);
> spin_unlock_bh(&sctp_assocs_id_lock);
> if (preload)
> idr_preload_end();
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-08 2:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-05 15:59 [PATCH net] sctp: use gfp insteaad of GFP_NOWAIT in idr_alloc_cyclic when sctp_assoc_set_id Xin Long
2016-03-05 16:42 ` Eric Dumazet
2016-03-06 5:52 ` Xin Long
2016-03-06 23:21 ` Eric Dumazet
2016-03-08 2:54 ` Xin Long
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.