| Message ID | 1453643800-86594-1-git-send-email-ardeleanalex@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On 01/24/2016 08:56 AM, Alexandru Ardelean wrote: > Seems that musl libc's default thread stack size is 80k, which > causes a segfault (stack overflow actually) when trying to add > a bridge (via the "ovs-vsctl add-br" command). > > OpenWRT has been switching to musl libc for a few months now. > So far, we've been using OVS with uClibc, so I did not catch this > earlier. > > This patch is a RFC: > - is this thread stack size sufficient ? > - is this approach acceptable ? > > In OpenWRT we'll use this patch since it works. > > Issue was found and fixed by Robert McKay. > I fine tuned his patch and will add it to our OVS package > in OpenWRT. > > Signed-off-by: Robert McKay <robert@mckay.com> > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> > --- > lib/ovs-thread.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c > index 7855b3a..e35ddba 100644 > --- a/lib/ovs-thread.c > +++ b/lib/ovs-thread.c > @@ -347,6 +347,7 @@ ovs_thread_create(const char *name, void *(*start)(void *), void *arg) > { > struct ovsthread_aux *aux; > pthread_t thread; > + pthread_attr_t attr; > int error; > > forbid_forking("multiple threads exist"); > @@ -358,7 +359,9 @@ ovs_thread_create(const char *name, void *(*start)(void *), void *arg) > aux->arg = arg; > ovs_strlcpy(aux->name, name, sizeof aux->name); > > - error = pthread_create(&thread, NULL, ovsthread_wrapper, aux); > + pthread_attr_init(&attr); > + pthread_attr_setstacksize(&attr, 128*1024); > + error = pthread_create(&thread, &attr, ovsthread_wrapper, aux); > if (error) { > ovs_abort(error, "pthread_create failed"); > } > I don't actually have feedback on the more important question of how best to deal with your issue, but fwiw, you need a pthread_attr_destroy() after pthread_create().
On Sun, Jan 24, 2016 at 11:24 PM, Russell Bryant <russell@ovn.org> wrote: > On 01/24/2016 08:56 AM, Alexandru Ardelean wrote: > > Seems that musl libc's default thread stack size is 80k, which > > causes a segfault (stack overflow actually) when trying to add > > a bridge (via the "ovs-vsctl add-br" command). > > > > OpenWRT has been switching to musl libc for a few months now. > > So far, we've been using OVS with uClibc, so I did not catch this > > earlier. > > > > This patch is a RFC: > > - is this thread stack size sufficient ? > > - is this approach acceptable ? > > > > In OpenWRT we'll use this patch since it works. > > > > Issue was found and fixed by Robert McKay. > > I fine tuned his patch and will add it to our OVS package > > in OpenWRT. > > > > Signed-off-by: Robert McKay <robert@mckay.com> > > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> > > --- > > lib/ovs-thread.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c > > index 7855b3a..e35ddba 100644 > > --- a/lib/ovs-thread.c > > +++ b/lib/ovs-thread.c > > @@ -347,6 +347,7 @@ ovs_thread_create(const char *name, void > *(*start)(void *), void *arg) > > { > > struct ovsthread_aux *aux; > > pthread_t thread; > > + pthread_attr_t attr; > > int error; > > > > forbid_forking("multiple threads exist"); > > @@ -358,7 +359,9 @@ ovs_thread_create(const char *name, void > *(*start)(void *), void *arg) > > aux->arg = arg; > > ovs_strlcpy(aux->name, name, sizeof aux->name); > > > > - error = pthread_create(&thread, NULL, ovsthread_wrapper, aux); > > + pthread_attr_init(&attr); > > + pthread_attr_setstacksize(&attr, 128*1024); > > + error = pthread_create(&thread, &attr, ovsthread_wrapper, aux); > > if (error) { > > ovs_abort(error, "pthread_create failed"); > > } > > > > I don't actually have feedback on the more important question of how > best to deal with your issue, but fwiw, you need a > pthread_attr_destroy() after pthread_create(). > > -- > Russell Bryant > Thanks for that. Re-spinned patch with your suggestion.
On Mon, Jan 25, 2016 at 10:13 AM, Alexandru Ardelean <ardeleanalex@gmail.com > wrote: > > > On Sun, Jan 24, 2016 at 11:24 PM, Russell Bryant <russell@ovn.org> wrote: > >> On 01/24/2016 08:56 AM, Alexandru Ardelean wrote: >> > Seems that musl libc's default thread stack size is 80k, which >> > causes a segfault (stack overflow actually) when trying to add >> > a bridge (via the "ovs-vsctl add-br" command). >> > >> > OpenWRT has been switching to musl libc for a few months now. >> > So far, we've been using OVS with uClibc, so I did not catch this >> > earlier. >> > >> > This patch is a RFC: >> > - is this thread stack size sufficient ? >> > - is this approach acceptable ? >> > >> > In OpenWRT we'll use this patch since it works. >> > >> > Issue was found and fixed by Robert McKay. >> > I fine tuned his patch and will add it to our OVS package >> > in OpenWRT. >> > >> > Signed-off-by: Robert McKay <robert@mckay.com> >> > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> >> > --- >> > lib/ovs-thread.c | 5 ++++- >> > 1 file changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c >> > index 7855b3a..e35ddba 100644 >> > --- a/lib/ovs-thread.c >> > +++ b/lib/ovs-thread.c >> > @@ -347,6 +347,7 @@ ovs_thread_create(const char *name, void >> *(*start)(void *), void *arg) >> > { >> > struct ovsthread_aux *aux; >> > pthread_t thread; >> > + pthread_attr_t attr; >> > int error; >> > >> > forbid_forking("multiple threads exist"); >> > @@ -358,7 +359,9 @@ ovs_thread_create(const char *name, void >> *(*start)(void *), void *arg) >> > aux->arg = arg; >> > ovs_strlcpy(aux->name, name, sizeof aux->name); >> > >> > - error = pthread_create(&thread, NULL, ovsthread_wrapper, aux); >> > + pthread_attr_init(&attr); >> > + pthread_attr_setstacksize(&attr, 128*1024); >> > + error = pthread_create(&thread, &attr, ovsthread_wrapper, aux); >> > if (error) { >> > ovs_abort(error, "pthread_create failed"); >> > } >> > >> >> I don't actually have feedback on the more important question of how >> best to deal with your issue, but fwiw, you need a >> pthread_attr_destroy() after pthread_create(). >> >> -- >> Russell Bryant >> > > Thanks for that. > Re-spinned patch with your suggestion. > > I've found that 128k is too small under some tests. Will try to increase this in increments of 128k (so next one is 256k) and run a few more tests. RFC for "is this approach acceptable ?" still stands.
On Sun, Jan 24, 2016 at 03:56:40PM +0200, Alexandru Ardelean wrote: > Seems that musl libc's default thread stack size is 80k, which > causes a segfault (stack overflow actually) when trying to add > a bridge (via the "ovs-vsctl add-br" command). > > OpenWRT has been switching to musl libc for a few months now. > So far, we've been using OVS with uClibc, so I did not catch this > earlier. > > This patch is a RFC: > - is this thread stack size sufficient ? > - is this approach acceptable ? 128 kB seems really small for anything other than embedded. The default on Linux/x86-32 is 2 MB. I think it's too small from an absolute perspective, too. If I run a command to find large stack allocations (on x86): objdump -d vswitchd/ovs-vswitchd|sed -n 's/.*sub.*\(0x[0-9a-f]*\),%esp/\1/p'|sort -gr|head then I see, ignoring negative subtractions (!), the following definitely or potentially problematic sizes: 0x3662c (~217 kB) 0x1a1ac (~104 kB) 0x1004c (~64 kB) which correspond to the following functions, respectively: recv_upcalls() dpif_netlink_operate__() nl_sock_recv__() So anything less than about 256 kB is definitely unsafe. I think that some of those uses might nest within each other, so I wouldn't recommend less than about 384 kB, honestly. But why does this question even come up? Don't you have a demand-paged stack? That is, why does allocating megabytes of virtual address space for stacks have a cost?
On Mon, Jan 25, 2016 at 03:16:22PM +0200, Alexandru Ardelean wrote: > On Mon, Jan 25, 2016 at 10:13 AM, Alexandru Ardelean <ardeleanalex@gmail.com > > wrote: > > > > > > > On Sun, Jan 24, 2016 at 11:24 PM, Russell Bryant <russell@ovn.org> wrote: > > > >> On 01/24/2016 08:56 AM, Alexandru Ardelean wrote: > >> > Seems that musl libc's default thread stack size is 80k, which > >> > causes a segfault (stack overflow actually) when trying to add > >> > a bridge (via the "ovs-vsctl add-br" command). > >> > > >> > OpenWRT has been switching to musl libc for a few months now. > >> > So far, we've been using OVS with uClibc, so I did not catch this > >> > earlier. > >> > > >> > This patch is a RFC: > >> > - is this thread stack size sufficient ? > >> > - is this approach acceptable ? > >> > > >> > In OpenWRT we'll use this patch since it works. > >> > > >> > Issue was found and fixed by Robert McKay. > >> > I fine tuned his patch and will add it to our OVS package > >> > in OpenWRT. > >> > > >> > Signed-off-by: Robert McKay <robert@mckay.com> > >> > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> > >> > --- > >> > lib/ovs-thread.c | 5 ++++- > >> > 1 file changed, 4 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c > >> > index 7855b3a..e35ddba 100644 > >> > --- a/lib/ovs-thread.c > >> > +++ b/lib/ovs-thread.c > >> > @@ -347,6 +347,7 @@ ovs_thread_create(const char *name, void > >> *(*start)(void *), void *arg) > >> > { > >> > struct ovsthread_aux *aux; > >> > pthread_t thread; > >> > + pthread_attr_t attr; > >> > int error; > >> > > >> > forbid_forking("multiple threads exist"); > >> > @@ -358,7 +359,9 @@ ovs_thread_create(const char *name, void > >> *(*start)(void *), void *arg) > >> > aux->arg = arg; > >> > ovs_strlcpy(aux->name, name, sizeof aux->name); > >> > > >> > - error = pthread_create(&thread, NULL, ovsthread_wrapper, aux); > >> > + pthread_attr_init(&attr); > >> > + pthread_attr_setstacksize(&attr, 128*1024); > >> > + error = pthread_create(&thread, &attr, ovsthread_wrapper, aux); > >> > if (error) { > >> > ovs_abort(error, "pthread_create failed"); > >> > } > >> > > >> > >> I don't actually have feedback on the more important question of how > >> best to deal with your issue, but fwiw, you need a > >> pthread_attr_destroy() after pthread_create(). > >> > >> -- > >> Russell Bryant > >> > > > > Thanks for that. > > Re-spinned patch with your suggestion. > > > > > I've found that 128k is too small under some tests. > Will try to increase this in increments of 128k (so next one is 256k) and > run a few more tests. > RFC for "is this approach acceptable ?" still stands. My reading of v2 is that it still uses 128 kB.
On Tue, Jan 26, 2016 at 8:34 AM, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Jan 25, 2016 at 03:16:22PM +0200, Alexandru Ardelean wrote: > > On Mon, Jan 25, 2016 at 10:13 AM, Alexandru Ardelean < > ardeleanalex@gmail.com > > > wrote: > > > > > > > > > > > On Sun, Jan 24, 2016 at 11:24 PM, Russell Bryant <russell@ovn.org> > wrote: > > > > > >> On 01/24/2016 08:56 AM, Alexandru Ardelean wrote: > > >> > Seems that musl libc's default thread stack size is 80k, which > > >> > causes a segfault (stack overflow actually) when trying to add > > >> > a bridge (via the "ovs-vsctl add-br" command). > > >> > > > >> > OpenWRT has been switching to musl libc for a few months now. > > >> > So far, we've been using OVS with uClibc, so I did not catch this > > >> > earlier. > > >> > > > >> > This patch is a RFC: > > >> > - is this thread stack size sufficient ? > > >> > - is this approach acceptable ? > > >> > > > >> > In OpenWRT we'll use this patch since it works. > > >> > > > >> > Issue was found and fixed by Robert McKay. > > >> > I fine tuned his patch and will add it to our OVS package > > >> > in OpenWRT. > > >> > > > >> > Signed-off-by: Robert McKay <robert@mckay.com> > > >> > Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> > > >> > --- > > >> > lib/ovs-thread.c | 5 ++++- > > >> > 1 file changed, 4 insertions(+), 1 deletion(-) > > >> > > > >> > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c > > >> > index 7855b3a..e35ddba 100644 > > >> > --- a/lib/ovs-thread.c > > >> > +++ b/lib/ovs-thread.c > > >> > @@ -347,6 +347,7 @@ ovs_thread_create(const char *name, void > > >> *(*start)(void *), void *arg) > > >> > { > > >> > struct ovsthread_aux *aux; > > >> > pthread_t thread; > > >> > + pthread_attr_t attr; > > >> > int error; > > >> > > > >> > forbid_forking("multiple threads exist"); > > >> > @@ -358,7 +359,9 @@ ovs_thread_create(const char *name, void > > >> *(*start)(void *), void *arg) > > >> > aux->arg = arg; > > >> > ovs_strlcpy(aux->name, name, sizeof aux->name); > > >> > > > >> > - error = pthread_create(&thread, NULL, ovsthread_wrapper, aux); > > >> > + pthread_attr_init(&attr); > > >> > + pthread_attr_setstacksize(&attr, 128*1024); > > >> > + error = pthread_create(&thread, &attr, ovsthread_wrapper, aux); > > >> > if (error) { > > >> > ovs_abort(error, "pthread_create failed"); > > >> > } > > >> > > > >> > > >> I don't actually have feedback on the more important question of how > > >> best to deal with your issue, but fwiw, you need a > > >> pthread_attr_destroy() after pthread_create(). > > >> > > >> -- > > >> Russell Bryant > > >> > > > > > > Thanks for that. > > > Re-spinned patch with your suggestion. > > > > > > > > I've found that 128k is too small under some tests. > > Will try to increase this in increments of 128k (so next one is 256k) and > > run a few more tests. > > RFC for "is this approach acceptable ?" still stands. > > My reading of v2 is that it still uses 128 kB. > Sorry to take so long to come back with some replies. Been working on other stuff too. To answer "But why does this question even come up?" This is in the context of musl libc which is a pretty basic embedded libc implementation. I'll reference their wiki page: http://wiki.musl-libc.org/wiki/Functional_differences_from_glibc#Thread_stack_size I don't know how to answer the other questions regarding demand-paged stack. And due to time constraints I would like to not go too deep into the details. Moving forward. Would it be an idea to add a ? #ifndef OVS_DEFAULT_THREAD_STACK_SIZE #define OVS_DEFAULT_THREAD_STACK_SIZE (2 * 1024 * 1024) #endif That way for our embedded musl libc case, we could add it to our CFLAGS. Btw, we've found OVS to be stable at 512k default thread stack size ; thanks for your input :) Thanks Alex
On Tue, Feb 02, 2016 at 01:13:16PM +0200, Alexandru Ardelean wrote: > Would it be an idea to add a ? > #ifndef OVS_DEFAULT_THREAD_STACK_SIZE > #define OVS_DEFAULT_THREAD_STACK_SIZE (2 * 1024 * 1024) > #endif > > That way for our embedded musl libc case, we could add it to our CFLAGS. What if we add a *minimum* stack size instead? If the default is higher than the minimum, I'd rather not reduce it. Something like this: #define OVS_MIN_THREAD_STACK_SIZE (512 * 1024) pthread_attr attr; size_t stacksize; pthread_attr_init(&attr); if (!pthread_attr_getstacksize(&attr, &stacksize) && stacksize < OVS_MIN_THREAD_STACK_SIZE) { pthread_attr_setstacksize(&attr, OVS_MIN_THREAD_STACK_SIZE); } although we'd probably want to report it if either pthread function returned an error.
On Tue, Feb 2, 2016 at 8:24 PM, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Feb 02, 2016 at 01:13:16PM +0200, Alexandru Ardelean wrote: > > Would it be an idea to add a ? > > #ifndef OVS_DEFAULT_THREAD_STACK_SIZE > > #define OVS_DEFAULT_THREAD_STACK_SIZE (2 * 1024 * 1024) > > #endif > > > > That way for our embedded musl libc case, we could add it to our CFLAGS. > > What if we add a *minimum* stack size instead? If the default is higher > than the minimum, I'd rather not reduce it. Something like this: > > #define OVS_MIN_THREAD_STACK_SIZE (512 * 1024) > pthread_attr attr; > size_t stacksize; > > pthread_attr_init(&attr); > if (!pthread_attr_getstacksize(&attr, &stacksize) > && stacksize < OVS_MIN_THREAD_STACK_SIZE) { > pthread_attr_setstacksize(&attr, OVS_MIN_THREAD_STACK_SIZE); > } > > although we'd probably want to report it if either pthread function > returned an error. > I like the approach. If it's fine with you, I'll re-spin a patch after a bit of testing
On Wed, Feb 03, 2016 at 05:45:30PM +0200, Alexandru Ardelean wrote: > On Tue, Feb 2, 2016 at 8:24 PM, Ben Pfaff <blp@ovn.org> wrote: > > > On Tue, Feb 02, 2016 at 01:13:16PM +0200, Alexandru Ardelean wrote: > > > Would it be an idea to add a ? > > > #ifndef OVS_DEFAULT_THREAD_STACK_SIZE > > > #define OVS_DEFAULT_THREAD_STACK_SIZE (2 * 1024 * 1024) > > > #endif > > > > > > That way for our embedded musl libc case, we could add it to our CFLAGS. > > > > What if we add a *minimum* stack size instead? If the default is higher > > than the minimum, I'd rather not reduce it. Something like this: > > > > #define OVS_MIN_THREAD_STACK_SIZE (512 * 1024) > > pthread_attr attr; > > size_t stacksize; > > > > pthread_attr_init(&attr); > > if (!pthread_attr_getstacksize(&attr, &stacksize) > > && stacksize < OVS_MIN_THREAD_STACK_SIZE) { > > pthread_attr_setstacksize(&attr, OVS_MIN_THREAD_STACK_SIZE); > > } > > > > although we'd probably want to report it if either pthread function > > returned an error. > > > > I like the approach. > If it's fine with you, I'll re-spin a patch after a bit of testing Sounds good to me.
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c index 7855b3a..e35ddba 100644 --- a/lib/ovs-thread.c +++ b/lib/ovs-thread.c @@ -347,6 +347,7 @@ ovs_thread_create(const char *name, void *(*start)(void *), void *arg) { struct ovsthread_aux *aux; pthread_t thread; + pthread_attr_t attr; int error; forbid_forking("multiple threads exist"); @@ -358,7 +359,9 @@ ovs_thread_create(const char *name, void *(*start)(void *), void *arg) aux->arg = arg; ovs_strlcpy(aux->name, name, sizeof aux->name); - error = pthread_create(&thread, NULL, ovsthread_wrapper, aux); + pthread_attr_init(&attr); + pthread_attr_setstacksize(&attr, 128*1024); + error = pthread_create(&thread, &attr, ovsthread_wrapper, aux); if (error) { ovs_abort(error, "pthread_create failed"); }