* [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