* [lttng-dev] [PATCH] Fix: call_rcu_thread() affinity failure @ 2015-06-29 22:56 Mathieu Desnoyers 2015-06-29 23:01 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Mathieu Desnoyers @ 2015-06-29 22:56 UTC (permalink / raw) Make call_rcu_thread() affine itself more persistently Currently, URCU simply fails if a call_rcu_thread() fails to affine itself. This is problematic when execution is constrained by cgroup and hotunplugged CPUs. This commit therefore makes call_rcu_thread() retry setting its affinity every 256 grace periods, but only if it detects that it migrated to a different CPU. Since sched_getcpu() is cheap on many architectures, this check is less costly than going through a system call. Reported-by: Michael Jeanson <mjeanson at efficios.com> Suggested-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com> --- urcu-call-rcu-impl.h | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h index 5cc02d9..b82a59b 100644 --- a/urcu-call-rcu-impl.h +++ b/urcu-call-rcu-impl.h @@ -45,6 +45,9 @@ #include "urcu/ref.h" #include "urcu-die.h" +#define SET_AFFINITY_CHECK_PERIOD (1U << 8) /* 256 */ +#define SET_AFFINITY_CHECK_PERIOD_MASK (SET_AFFINITY_CHECK_PERIOD - 1) + /* Data structure that identifies a call_rcu thread. */ struct call_rcu_data { @@ -62,6 +65,7 @@ struct call_rcu_data { unsigned long qlen; /* maintained for debugging. */ pthread_t tid; int cpu_affinity; + unsigned long gp_count; struct cds_list_head list; } __attribute__((aligned(CAA_CACHE_LINE_SIZE))); @@ -203,22 +207,42 @@ static void call_rcu_unlock(pthread_mutex_t *pmp) urcu_die(ret); } +/* + * Periodically retry setting CPU affinity if we migrate. + * Losing affinity can be caused by CPU hotunplug/hotplug, or by + * cpuset(7). + */ #if HAVE_SCHED_SETAFFINITY static int set_thread_cpu_affinity(struct call_rcu_data *crdp) { cpu_set_t mask; + int ret; if (crdp->cpu_affinity < 0) return 0; + if (++crdp->gp_count & SET_AFFINITY_CHECK_PERIOD_MASK) + return 0; + if (urcu_sched_getcpu() != crdp->cpu_affinity) + return 0; CPU_ZERO(&mask); CPU_SET(crdp->cpu_affinity, &mask); #if SCHED_SETAFFINITY_ARGS == 2 - return sched_setaffinity(0, &mask); + ret = sched_setaffinity(0, &mask); #else - return sched_setaffinity(0, sizeof(mask), &mask); + ret = sched_setaffinity(0, sizeof(mask), &mask); #endif + /* + * EINVAL is fine: can be caused by hotunplugged CPUs, or by + * cpuset(7). This is why we should always retry is we detect + * migration. + */ + if (ret && errno == EINVAL) { + ret = 0; + errno = 0; + } + return ret; } #else static @@ -275,10 +299,8 @@ static void *call_rcu_thread(void *arg) unsigned long cbcount; struct call_rcu_data *crdp = (struct call_rcu_data *) arg; int rt = !!(uatomic_read(&crdp->flags) & URCU_CALL_RCU_RT); - int ret; - ret = set_thread_cpu_affinity(crdp); - if (ret) + if (set_thread_cpu_affinity(crdp)) urcu_die(errno); /* @@ -298,6 +320,9 @@ static void *call_rcu_thread(void *arg) struct cds_wfcq_node *cbs, *cbs_tmp_n; enum cds_wfcq_ret splice_ret; + if (set_thread_cpu_affinity(crdp)) + urcu_die(errno); + if (uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) { /* * Pause requested. Become quiescent: remove @@ -391,6 +416,7 @@ static void call_rcu_data_init(struct call_rcu_data **crdpp, crdp->flags = flags; cds_list_add(&crdp->list, &call_rcu_data_list); crdp->cpu_affinity = cpu_affinity; + crdp->gp_count = 0; cmm_smp_mb(); /* Structure initialized before pointer is planted. */ *crdpp = crdp; ret = pthread_create(&crdp->tid, NULL, call_rcu_thread, crdp); -- 2.1.4 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [lttng-dev] [PATCH] Fix: call_rcu_thread() affinity failure 2015-06-29 22:56 [lttng-dev] [PATCH] Fix: call_rcu_thread() affinity failure Mathieu Desnoyers @ 2015-06-29 23:01 ` Paul E. McKenney 2015-06-29 23:06 ` Mathieu Desnoyers 0 siblings, 1 reply; 4+ messages in thread From: Paul E. McKenney @ 2015-06-29 23:01 UTC (permalink / raw) On Mon, Jun 29, 2015 at 06:56:34PM -0400, Mathieu Desnoyers wrote: > Make call_rcu_thread() affine itself more persistently > > Currently, URCU simply fails if a call_rcu_thread() fails to affine > itself. This is problematic when execution is constrained by cgroup > and hotunplugged CPUs. This commit therefore makes call_rcu_thread() > retry setting its affinity every 256 grace periods, but only if it > detects that it migrated to a different CPU. Since sched_getcpu() is > cheap on many architectures, this check is less costly than going > through a system call. > > Reported-by: Michael Jeanson <mjeanson at efficios.com> > Suggested-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com> A couple of issues, but otherwise good. (They might even be issues with your code rather than my eyes, you never know!) Thanx, Paul > --- > urcu-call-rcu-impl.h | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h > index 5cc02d9..b82a59b 100644 > --- a/urcu-call-rcu-impl.h > +++ b/urcu-call-rcu-impl.h > @@ -45,6 +45,9 @@ > #include "urcu/ref.h" > #include "urcu-die.h" > > +#define SET_AFFINITY_CHECK_PERIOD (1U << 8) /* 256 */ > +#define SET_AFFINITY_CHECK_PERIOD_MASK (SET_AFFINITY_CHECK_PERIOD - 1) > + > /* Data structure that identifies a call_rcu thread. */ > > struct call_rcu_data { > @@ -62,6 +65,7 @@ struct call_rcu_data { > unsigned long qlen; /* maintained for debugging. */ > pthread_t tid; > int cpu_affinity; > + unsigned long gp_count; > struct cds_list_head list; > } __attribute__((aligned(CAA_CACHE_LINE_SIZE))); > > @@ -203,22 +207,42 @@ static void call_rcu_unlock(pthread_mutex_t *pmp) > urcu_die(ret); > } > > +/* > + * Periodically retry setting CPU affinity if we migrate. > + * Losing affinity can be caused by CPU hotunplug/hotplug, or by > + * cpuset(7). > + */ > #if HAVE_SCHED_SETAFFINITY > static > int set_thread_cpu_affinity(struct call_rcu_data *crdp) > { > cpu_set_t mask; > + int ret; > > if (crdp->cpu_affinity < 0) > return 0; > + if (++crdp->gp_count & SET_AFFINITY_CHECK_PERIOD_MASK) > + return 0; > + if (urcu_sched_getcpu() != crdp->cpu_affinity) Don't we want "==" here instead of "!="? > + return 0; > > CPU_ZERO(&mask); > CPU_SET(crdp->cpu_affinity, &mask); > #if SCHED_SETAFFINITY_ARGS == 2 > - return sched_setaffinity(0, &mask); > + ret = sched_setaffinity(0, &mask); > #else > - return sched_setaffinity(0, sizeof(mask), &mask); > + ret = sched_setaffinity(0, sizeof(mask), &mask); > #endif > + /* > + * EINVAL is fine: can be caused by hotunplugged CPUs, or by > + * cpuset(7). This is why we should always retry is we detect s/is we detect/if we detect/ > + * migration. > + */ > + if (ret && errno == EINVAL) { > + ret = 0; > + errno = 0; > + } > + return ret; > } > #else > static > @@ -275,10 +299,8 @@ static void *call_rcu_thread(void *arg) > unsigned long cbcount; > struct call_rcu_data *crdp = (struct call_rcu_data *) arg; > int rt = !!(uatomic_read(&crdp->flags) & URCU_CALL_RCU_RT); > - int ret; > > - ret = set_thread_cpu_affinity(crdp); > - if (ret) > + if (set_thread_cpu_affinity(crdp)) > urcu_die(errno); > > /* > @@ -298,6 +320,9 @@ static void *call_rcu_thread(void *arg) > struct cds_wfcq_node *cbs, *cbs_tmp_n; > enum cds_wfcq_ret splice_ret; > > + if (set_thread_cpu_affinity(crdp)) > + urcu_die(errno); > + > if (uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) { > /* > * Pause requested. Become quiescent: remove > @@ -391,6 +416,7 @@ static void call_rcu_data_init(struct call_rcu_data **crdpp, > crdp->flags = flags; > cds_list_add(&crdp->list, &call_rcu_data_list); > crdp->cpu_affinity = cpu_affinity; > + crdp->gp_count = 0; > cmm_smp_mb(); /* Structure initialized before pointer is planted. */ > *crdpp = crdp; > ret = pthread_create(&crdp->tid, NULL, call_rcu_thread, crdp); > -- > 2.1.4 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [lttng-dev] [PATCH] Fix: call_rcu_thread() affinity failure 2015-06-29 23:01 ` Paul E. McKenney @ 2015-06-29 23:06 ` Mathieu Desnoyers 2015-06-29 23:15 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Mathieu Desnoyers @ 2015-06-29 23:06 UTC (permalink / raw) ----- On Jun 29, 2015, at 7:01 PM, Paul E. McKenney paulmck at linux.vnet.ibm.com wrote: > On Mon, Jun 29, 2015 at 06:56:34PM -0400, Mathieu Desnoyers wrote: >> Make call_rcu_thread() affine itself more persistently >> >> Currently, URCU simply fails if a call_rcu_thread() fails to affine >> itself. This is problematic when execution is constrained by cgroup >> and hotunplugged CPUs. This commit therefore makes call_rcu_thread() >> retry setting its affinity every 256 grace periods, but only if it >> detects that it migrated to a different CPU. Since sched_getcpu() is >> cheap on many architectures, this check is less costly than going >> through a system call. >> >> Reported-by: Michael Jeanson <mjeanson at efficios.com> >> Suggested-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com> >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com> > > A couple of issues, but otherwise good. (They might even be issues > with your code rather than my eyes, you never know!) Your eyes are very good indeed. Fixing those 2 nits, and adding your Acked-by. Thanks! Mathieu > > Thanx, Paul > >> --- >> urcu-call-rcu-impl.h | 36 +++++++++++++++++++++++++++++++----- >> 1 file changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h >> index 5cc02d9..b82a59b 100644 >> --- a/urcu-call-rcu-impl.h >> +++ b/urcu-call-rcu-impl.h >> @@ -45,6 +45,9 @@ >> #include "urcu/ref.h" >> #include "urcu-die.h" >> >> +#define SET_AFFINITY_CHECK_PERIOD (1U << 8) /* 256 */ >> +#define SET_AFFINITY_CHECK_PERIOD_MASK (SET_AFFINITY_CHECK_PERIOD - 1) >> + >> /* Data structure that identifies a call_rcu thread. */ >> >> struct call_rcu_data { >> @@ -62,6 +65,7 @@ struct call_rcu_data { >> unsigned long qlen; /* maintained for debugging. */ >> pthread_t tid; >> int cpu_affinity; >> + unsigned long gp_count; >> struct cds_list_head list; >> } __attribute__((aligned(CAA_CACHE_LINE_SIZE))); >> >> @@ -203,22 +207,42 @@ static void call_rcu_unlock(pthread_mutex_t *pmp) >> urcu_die(ret); >> } >> >> +/* >> + * Periodically retry setting CPU affinity if we migrate. >> + * Losing affinity can be caused by CPU hotunplug/hotplug, or by >> + * cpuset(7). >> + */ >> #if HAVE_SCHED_SETAFFINITY >> static >> int set_thread_cpu_affinity(struct call_rcu_data *crdp) >> { >> cpu_set_t mask; >> + int ret; >> >> if (crdp->cpu_affinity < 0) >> return 0; >> + if (++crdp->gp_count & SET_AFFINITY_CHECK_PERIOD_MASK) >> + return 0; >> + if (urcu_sched_getcpu() != crdp->cpu_affinity) > > Don't we want "==" here instead of "!="? > >> + return 0; >> >> CPU_ZERO(&mask); >> CPU_SET(crdp->cpu_affinity, &mask); >> #if SCHED_SETAFFINITY_ARGS == 2 >> - return sched_setaffinity(0, &mask); >> + ret = sched_setaffinity(0, &mask); >> #else >> - return sched_setaffinity(0, sizeof(mask), &mask); >> + ret = sched_setaffinity(0, sizeof(mask), &mask); >> #endif >> + /* >> + * EINVAL is fine: can be caused by hotunplugged CPUs, or by >> + * cpuset(7). This is why we should always retry is we detect > > s/is we detect/if we detect/ > >> + * migration. >> + */ >> + if (ret && errno == EINVAL) { >> + ret = 0; >> + errno = 0; >> + } >> + return ret; >> } >> #else >> static >> @@ -275,10 +299,8 @@ static void *call_rcu_thread(void *arg) >> unsigned long cbcount; >> struct call_rcu_data *crdp = (struct call_rcu_data *) arg; >> int rt = !!(uatomic_read(&crdp->flags) & URCU_CALL_RCU_RT); >> - int ret; >> >> - ret = set_thread_cpu_affinity(crdp); >> - if (ret) >> + if (set_thread_cpu_affinity(crdp)) >> urcu_die(errno); >> >> /* >> @@ -298,6 +320,9 @@ static void *call_rcu_thread(void *arg) >> struct cds_wfcq_node *cbs, *cbs_tmp_n; >> enum cds_wfcq_ret splice_ret; >> >> + if (set_thread_cpu_affinity(crdp)) >> + urcu_die(errno); >> + >> if (uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) { >> /* >> * Pause requested. Become quiescent: remove >> @@ -391,6 +416,7 @@ static void call_rcu_data_init(struct call_rcu_data **crdpp, >> crdp->flags = flags; >> cds_list_add(&crdp->list, &call_rcu_data_list); >> crdp->cpu_affinity = cpu_affinity; >> + crdp->gp_count = 0; >> cmm_smp_mb(); /* Structure initialized before pointer is planted. */ >> *crdpp = crdp; >> ret = pthread_create(&crdp->tid, NULL, call_rcu_thread, crdp); >> -- >> 2.1.4 -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* [lttng-dev] [PATCH] Fix: call_rcu_thread() affinity failure 2015-06-29 23:06 ` Mathieu Desnoyers @ 2015-06-29 23:15 ` Paul E. McKenney 0 siblings, 0 replies; 4+ messages in thread From: Paul E. McKenney @ 2015-06-29 23:15 UTC (permalink / raw) On Mon, Jun 29, 2015 at 11:06:15PM +0000, Mathieu Desnoyers wrote: > ----- On Jun 29, 2015, at 7:01 PM, Paul E. McKenney paulmck at linux.vnet.ibm.com wrote: > > > On Mon, Jun 29, 2015 at 06:56:34PM -0400, Mathieu Desnoyers wrote: > >> Make call_rcu_thread() affine itself more persistently > >> > >> Currently, URCU simply fails if a call_rcu_thread() fails to affine > >> itself. This is problematic when execution is constrained by cgroup > >> and hotunplugged CPUs. This commit therefore makes call_rcu_thread() > >> retry setting its affinity every 256 grace periods, but only if it > >> detects that it migrated to a different CPU. Since sched_getcpu() is > >> cheap on many architectures, this check is less costly than going > >> through a system call. > >> > >> Reported-by: Michael Jeanson <mjeanson at efficios.com> > >> Suggested-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com> > >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com> > > > > A couple of issues, but otherwise good. (They might even be issues > > with your code rather than my eyes, you never know!) > > Your eyes are very good indeed. Fixing those 2 nits, and adding > your Acked-by. Very good! That said, I would feel better about my eyes had they not inserted the bug being fixed. ;-) Thanx, Paul > Thanks! > > Mathieu > > > > > Thanx, Paul > > > >> --- > >> urcu-call-rcu-impl.h | 36 +++++++++++++++++++++++++++++++----- > >> 1 file changed, 31 insertions(+), 5 deletions(-) > >> > >> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h > >> index 5cc02d9..b82a59b 100644 > >> --- a/urcu-call-rcu-impl.h > >> +++ b/urcu-call-rcu-impl.h > >> @@ -45,6 +45,9 @@ > >> #include "urcu/ref.h" > >> #include "urcu-die.h" > >> > >> +#define SET_AFFINITY_CHECK_PERIOD (1U << 8) /* 256 */ > >> +#define SET_AFFINITY_CHECK_PERIOD_MASK (SET_AFFINITY_CHECK_PERIOD - 1) > >> + > >> /* Data structure that identifies a call_rcu thread. */ > >> > >> struct call_rcu_data { > >> @@ -62,6 +65,7 @@ struct call_rcu_data { > >> unsigned long qlen; /* maintained for debugging. */ > >> pthread_t tid; > >> int cpu_affinity; > >> + unsigned long gp_count; > >> struct cds_list_head list; > >> } __attribute__((aligned(CAA_CACHE_LINE_SIZE))); > >> > >> @@ -203,22 +207,42 @@ static void call_rcu_unlock(pthread_mutex_t *pmp) > >> urcu_die(ret); > >> } > >> > >> +/* > >> + * Periodically retry setting CPU affinity if we migrate. > >> + * Losing affinity can be caused by CPU hotunplug/hotplug, or by > >> + * cpuset(7). > >> + */ > >> #if HAVE_SCHED_SETAFFINITY > >> static > >> int set_thread_cpu_affinity(struct call_rcu_data *crdp) > >> { > >> cpu_set_t mask; > >> + int ret; > >> > >> if (crdp->cpu_affinity < 0) > >> return 0; > >> + if (++crdp->gp_count & SET_AFFINITY_CHECK_PERIOD_MASK) > >> + return 0; > >> + if (urcu_sched_getcpu() != crdp->cpu_affinity) > > > > Don't we want "==" here instead of "!="? > > > >> + return 0; > >> > >> CPU_ZERO(&mask); > >> CPU_SET(crdp->cpu_affinity, &mask); > >> #if SCHED_SETAFFINITY_ARGS == 2 > >> - return sched_setaffinity(0, &mask); > >> + ret = sched_setaffinity(0, &mask); > >> #else > >> - return sched_setaffinity(0, sizeof(mask), &mask); > >> + ret = sched_setaffinity(0, sizeof(mask), &mask); > >> #endif > >> + /* > >> + * EINVAL is fine: can be caused by hotunplugged CPUs, or by > >> + * cpuset(7). This is why we should always retry is we detect > > > > s/is we detect/if we detect/ > > > >> + * migration. > >> + */ > >> + if (ret && errno == EINVAL) { > >> + ret = 0; > >> + errno = 0; > >> + } > >> + return ret; > >> } > >> #else > >> static > >> @@ -275,10 +299,8 @@ static void *call_rcu_thread(void *arg) > >> unsigned long cbcount; > >> struct call_rcu_data *crdp = (struct call_rcu_data *) arg; > >> int rt = !!(uatomic_read(&crdp->flags) & URCU_CALL_RCU_RT); > >> - int ret; > >> > >> - ret = set_thread_cpu_affinity(crdp); > >> - if (ret) > >> + if (set_thread_cpu_affinity(crdp)) > >> urcu_die(errno); > >> > >> /* > >> @@ -298,6 +320,9 @@ static void *call_rcu_thread(void *arg) > >> struct cds_wfcq_node *cbs, *cbs_tmp_n; > >> enum cds_wfcq_ret splice_ret; > >> > >> + if (set_thread_cpu_affinity(crdp)) > >> + urcu_die(errno); > >> + > >> if (uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) { > >> /* > >> * Pause requested. Become quiescent: remove > >> @@ -391,6 +416,7 @@ static void call_rcu_data_init(struct call_rcu_data **crdpp, > >> crdp->flags = flags; > >> cds_list_add(&crdp->list, &call_rcu_data_list); > >> crdp->cpu_affinity = cpu_affinity; > >> + crdp->gp_count = 0; > >> cmm_smp_mb(); /* Structure initialized before pointer is planted. */ > >> *crdpp = crdp; > >> ret = pthread_create(&crdp->tid, NULL, call_rcu_thread, crdp); > >> -- > >> 2.1.4 > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-29 23:15 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-29 22:56 [lttng-dev] [PATCH] Fix: call_rcu_thread() affinity failure Mathieu Desnoyers 2015-06-29 23:01 ` Paul E. McKenney 2015-06-29 23:06 ` Mathieu Desnoyers 2015-06-29 23:15 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox