On Thu, 2022-04-28 at 13:58 -0700, Alexei Starovoitov wrote: > On Thu, Apr 28, 2022 at 12:15 PM Delyan Kratunov <delyank@xxxxxx> wrote: > > > > On Thu, 2022-04-28 at 11:19 -0700, Andrii Nakryiko wrote: > > > On Thu, Apr 28, 2022 at 9:54 AM Delyan Kratunov <delyank@xxxxxx> wrote: > > > > > > > > uprobes work by raising a trap, setting a task flag from within the > > > > interrupt handler, and processing the actual work for the uprobe on the > > > > way back to userspace. As a result, uprobe handlers already execute in a > > > > user context. The primary obstacle to sleepable bpf uprobe programs is > > > > therefore on the bpf side. > > > > > > > > Namely, the bpf_prog_array attached to the uprobe is protected by normal > > > > rcu and runs with disabled preemption. In order for uprobe bpf programs > > > > to become actually sleepable, we need it to be protected by the tasks_trace > > > > rcu flavor instead (and kfree() called after a corresponding grace period). > > > > > > > > One way to achieve this is by tracking an array-has-contained-sleepable-prog > > > > flag in bpf_prog_array and switching rcu flavors based on it. However, this > > > > is deemed somewhat unwieldly and the rcu flavor transition would be hard > > > > to reason about. > > > > > > > > Instead, based on Alexei's proposal, we change the free path for > > > > bpf_prog_array to chain a tasks_trace and normal grace periods > > > > one after the other. Users who iterate under tasks_trace read section would > > > > be safe, as would users who iterate under normal read sections (from > > > > non-sleepable locations). The downside is that we take the tasks_trace latency > > > > unconditionally but that's deemed acceptable under expected workloads. > > > > > > One example where this actually can become a problem is cgroup BPF > > > programs. There you can make single attachment to root cgroup, but it > > > will create one "effective" prog_array for each descendant (and will > > > keep destroying and creating them as child cgroups are created). So > > > there is this invisible multiplier which we can't really control. > > > > > > So paying the (however small, but) price of call_rcu_tasks_trace() in > > > bpf_prog_array_free() which is used for purely non-sleepable cases > > > seems unfortunate. But I think an alternative to tracking this "has > > > sleepable" bit on a per bpf_prog_array case is to have > > > bpf_prog_array_free_sleepable() implementation independent of > > > bpf_prog_array_free() itself and call that sleepable variant from > > > uprobe detach handler, limiting the impact to things that actually > > > might be running as sleepable and which most likely won't churn > > > through a huge amount of arrays. WDYT? > > > > Honestly, I don't like the idea of having two different APIs, where if you use the > > wrong one, the program would only fail in rare and undebuggable circumstances. > > > > If we need specialization (and I'm not convinced we do - what's the rate of cgroup > > creation that we can sustain?), we should track that in the bpf_prog_array itself. We > > can have the allocation path set a flag and branch on it in free() to determine the > > necessary grace periods. > > I think what Andrii is proposing is to leave bpf_prog_array_free() as-is > and introduce new bpf_prog_array_free_sleepable() (the way it is in > this patch) and call it from 2 places in kernel/trace/bpf_trace.c only. > This way cgroup won't be affected. > > The rcu_trace protection here applies to prog_array itself. > Normal progs are still rcu, sleepable progs are rcu_trace. > Regardless whether they're called via trampoline or this new prog_array. Right, I understand the proposal. My objection is that if tomorrow someone mistakenly keeps using bpf_prog_array_free when they actually need bpf_prog_array_free_sleepable, the resulting bug is really difficult to find and reason about. I can make it correct in this patch series but *keeping* things correct going forward will be harder when there's two free variants. Instead, we can have a ARRAY_USE_TRACE_RCU flag which automatically chains the grace periods inside bpf_prog_array_free. That way we eliminate potential future confusion and instead of introducing subtle rcu bugs, at worst you can incur a performance penalty by using the flag where you don't need it. If we spend the time to one-way flip the flag only when you actually insert a sleepable program into the array, even that penalty is eliminated. The cost of this tradeoff in maintainability is 4 bytes more per array object (less if we pack it but that's a worse idea performance-wise). Given how much we already allocate, I think that's fair but I'm happy to discuss other less foot-gun-y ideas if the memory usage is a constraint.
