From mboxrd@z Thu Jan 1 00:00:00 1970 From: paulmck@linux.vnet.ibm.com (Paul E. McKenney) Date: Mon, 29 Jun 2015 16:15:30 -0700 Subject: [lttng-dev] [PATCH] Fix: call_rcu_thread() affinity failure In-Reply-To: <1917360537.1139.1435619175624.JavaMail.zimbra@efficios.com> References: <1435618594-16125-1-git-send-email-mathieu.desnoyers@efficios.com> <20150629230148.GW3717@linux.vnet.ibm.com> <1917360537.1139.1435619175624.JavaMail.zimbra@efficios.com> Message-ID: <20150629231530.GY3717@linux.vnet.ibm.com> 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 > >> Suggested-by: Paul E. McKenney > >> Signed-off-by: Mathieu Desnoyers > > > > 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 >