Messages in this thread | | | Date | Tue, 10 Jul 2018 09:57:44 -0700 | From | Joel Fernandes <> | Subject | Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command |
| |
On Tue, Jul 10, 2018 at 09:42:12AM -0700, Paul E. McKenney wrote: > On Mon, Jul 09, 2018 at 10:13:47PM -0700, Joel Fernandes wrote: > > On Sun, Jul 08, 2018 at 04:54:38PM -0400, Mathieu Desnoyers wrote: > > > ----- On Jul 7, 2018, at 4:33 PM, Joel Fernandes joelaf@google.com wrote: > > > > > > > On Fri, Jul 06, 2018 at 07:54:28PM -0700, Alexei Starovoitov wrote: > > > >> On Fri, Jul 06, 2018 at 06:56:16PM -0700, Daniel Colascione wrote: > > > >> > BPF_SYNCHRONIZE waits for any BPF programs active at the time of > > > >> > BPF_SYNCHRONIZE to complete, allowing userspace to ensure atomicity of > > > >> > RCU data structure operations with respect to active programs. For > > > >> > example, userspace can update a map->map entry to point to a new map, > > > >> > use BPF_SYNCHRONIZE to wait for any BPF programs using the old map to > > > >> > complete, and then drain the old map without fear that BPF programs > > > >> > may still be updating it. > > > >> > > > > >> > Signed-off-by: Daniel Colascione <dancol@google.com> > > > >> > --- > > > >> > include/uapi/linux/bpf.h | 1 + > > > >> > kernel/bpf/syscall.c | 14 ++++++++++++++ > > > >> > 2 files changed, 15 insertions(+) > > > >> > > > > >> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > >> > index b7db3261c62d..4365c50e8055 100644 > > > >> > --- a/include/uapi/linux/bpf.h > > > >> > +++ b/include/uapi/linux/bpf.h > > > >> > @@ -98,6 +98,7 @@ enum bpf_cmd { > > > >> > BPF_BTF_LOAD, > > > >> > BPF_BTF_GET_FD_BY_ID, > > > >> > BPF_TASK_FD_QUERY, > > > >> > + BPF_SYNCHRONIZE, > > > >> > }; > > > >> > > > > >> > enum bpf_map_type { > > > >> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > >> > index d10ecd78105f..60ec7811846e 100644 > > > >> > --- a/kernel/bpf/syscall.c > > > >> > +++ b/kernel/bpf/syscall.c > > > >> > @@ -2272,6 +2272,20 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, > > > >> > uattr, unsigned int, siz > > > >> > if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN)) > > > >> > return -EPERM; > > > >> > > > > >> > + if (cmd == BPF_SYNCHRONIZE) { > > > >> > + if (uattr != NULL || size != 0) > > > >> > + return -EINVAL; > > > >> > + err = security_bpf(cmd, NULL, 0); > > > >> > + if (err < 0) > > > >> > + return err; > > > >> > + /* BPF programs are run with preempt disabled, so > > > >> > + * synchronize_sched is sufficient even with > > > >> > + * RCU_PREEMPT. > > > >> > + */ > > > >> > + synchronize_sched(); > > > >> > + return 0; > > > >> > > > >> I don't think it's necessary. sys_membarrier() can do this already > > > >> and some folks use it exactly for this use case. > > > > > > > > Alexei, the use of sys_membarrier for this purpose seems kind of weird to me > > > > though. No where does the manpage say membarrier should be implemented this > > > > way so what happens if the implementation changes? > > > > > > > > Further, membarrier manpage says that a memory barrier should be matched with > > > > a matching barrier. In this use case there is no matching barrier, so it > > > > makes it weirder. > > > > > > > > Lastly, sys_membarrier seems will not work on nohz-full systems, so its a bit > > > > fragile to depend on it for this? > > > > > > > > case MEMBARRIER_CMD_GLOBAL: > > > > /* MEMBARRIER_CMD_GLOBAL is not compatible with nohz_full. */ > > > > if (tick_nohz_full_enabled()) > > > > return -EINVAL; > > > > if (num_online_cpus() > 1) > > > > synchronize_sched(); > > > > return 0; > > > > > > > > > > > > Adding Mathieu as well who I believe is author/maintainer of membarrier. > > > > > > See commit 907565337 > > > "Fix: Disable sys_membarrier when nohz_full is enabled" > > > > > > "Userspace applications should be allowed to expect the membarrier system > > > call with MEMBARRIER_CMD_SHARED command to issue memory barriers on > > > nohz_full CPUs, but synchronize_sched() does not take those into > > > account." > > > > > > So AFAIU you'd want to re-use membarrier to issue synchronize_sched, and you > > > only care about kernel preempt off critical sections. > > > > Mathieu, Thanks a lot for your reply. I understand what you said and agree > > with you. Slight OT, but I tried to go back to first principles and > > understand how membarrier() uses synchronize_sched() for the "slow path" and > > it didn't make immediate sense to me. Let me clarify my dillema.. > > > > My understanding is membarrier's MEMBARRIER_CMD_GLOBAL will employ > > synchronize_sched to make sure all other CPUs aren't executing anymore in an > > section of usercode that happen to be accessing memory that was written to > > before the membarrier call was made. To do this, the system call will use > > synchronize_sched to try to guarantee that all user-mode execution that > > started before the membarrier call would be completed when the membarrier > > call returns. This guarantees that without using a real memory barrier on the > > "fast path", things work just fine and everyone wins. > > > > But, going through RCU code, I see that a "RCU-sched quiecent state" on a CPU > > may be reached when the CPU receives a timer tick while executing in user > > mode: > > > > void rcu_check_callbacks(int user) > > { > > trace_rcu_utilization(TPS("Start scheduler-tick")); > > increment_cpu_stall_ticks(); > > if (user || rcu_is_cpu_rrupt_from_idle()) { > > [...] > > rcu_sched_qs(); > > rcu_bh_qs(); > > > > The problem I see is the CPU could be executing usermode code at the time of > > the RCU sched-QS. This IMO is enough reason for synchronize_sched() to > > return, because the CPU in question just reported a QS (assuming all other > > CPUs also happen to do so if they needed to). > > This scenario will have inserted the needed smp_mb() into the userspace > instruction execution stream, as is required by the sys_membarrier > use cases.
Oh ok, that makes sense!
> > Then I am wondering how does the membarrier call even work, the tick could > > very well have interrupted the CPU while it was executing usermode code in > > the middle of a set of instructions performing memory accesses. Reporting a > > quiescent state at such an inopportune time would cause the membarrier call > > to prematurely return, no? Sorry if I missed something. > > One way to think of sys_membarrier() is as something that promotes a > barrier() to an smp_mb(). This barrier then separates the target CPU's > accesses that the caller saw before the sys_membarrier() from that same > CPU's accesses that the caller will see after the sys_membarrier().
Got it!
> > The other question I have is about the whole "nohz-full doesn't work" thing. > > I didn't fully understand why. RCU is already tracking the state of nohz-full > > CPUs because the rcu dynticks code in (kernel/rcu/tree.c) monitors > > transitions to and from usermode even if the timer tick is turned off. So why > > would it not work? > > In the nohz_full case, there is no need for sys_membarrier()'s call to > synchronize_sched() to interact directly with the nohz_full CPU. It > can instead look at the target CPU's dyntick-idle state, and that state > would potentially have been set in the dim distant past, thus having > no effect on the target CPU's current execution.
In nohz-idle case though, there's nothing to promote the barrier() to smp_mb() if you were to purely look at the dynticks-idle state on the nohz-full CPU executing in user mode?
So then it makes sense to me now that nohz-full needs something to IPI that CPU inorder to enforce the needed memory barrier and pure synchronize_sched() wouldn't work. So then makes me think the expedited versions of synchronize_sched should be able to do the job but I could off on a different track..
Thanks a lot,
-Joel
| |