On Wed, Nov 24, 2021 at 12:14:29AM +0100, KP Singh wrote: > On Tue, Nov 23, 2021 at 11:30 PM Martin KaFai Lau <kafai@xxxxxx> wrote: > > > > On Tue, Nov 23, 2021 at 10:22:04AM -0800, Paul E. McKenney wrote: > > > On Tue, Nov 23, 2021 at 06:11:14PM +0100, KP Singh wrote: > > > > On Thu, Sep 2, 2021 at 6:45 AM Martin KaFai Lau <kafai@xxxxxx> wrote: > > > > > I think the global lock will be an issue for the current non-sleepable > > > > > netdev bpf-prog which could be triggered by external traffic, so a flag > > > > > is needed here to provide a fast path. I suspect other non-prealloc map > > > > > may need it in the future, so probably > > > > > s/BPF_F_SLEEPABLE_STORAGE/BPF_F_SLEEPABLE/ instead. > > > > > > > > I was re-working the patches and had a couple of questions. > > > > > > > > There are two data structures that get freed under RCU here: > > > > > > > > struct bpf_local_storage > > > > struct bpf_local_storage_selem > > > > > > > > We can choose to free the bpf_local_storage_selem under > > > > call_rcu_tasks_trace based on > > > > whether the map it belongs to is sleepable with something like: > > > > > > > > if (selem->sdata.smap->map.map_flags & BPF_F_SLEEPABLE_STORAGE) > > Paul's current work (mentioned by his previous email) will improve the > > performance of call_rcu_tasks_trace, so it probably can avoid the > > new BPF_F_SLEEPABLE flag and make it easier to use. > > > > > > call_rcu_tasks_trace(&selem->rcu, bpf_selem_free_rcu); > > > > else > > > > kfree_rcu(selem, rcu); > > > > > > > > Questions: > > > > > > > > * Can we free bpf_local_storage under kfree_rcu by ensuring it's > > > > always accessed in a classical RCU critical section? > > >> Or maybe I am missing something and this also needs to be freed > > > > under trace RCU if any of the selems are from a sleepable map. > > In the inode_storage_lookup() of this patch: > > > > +#define bpf_local_storage_rcu_lock_held() \ > > + (rcu_read_lock_held() || rcu_read_lock_trace_held() || \ > > + rcu_read_lock_bh_held()) > > > > @@ -44,7 +45,8 @@ static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode, > > if (!bsb) > > return NULL; > > > > - inode_storage = rcu_dereference(bsb->storage); > > + inode_storage = rcu_dereference_protected(bsb->storage, > > + bpf_local_storage_rcu_lock_held()); > > > > Thus, it is not always in classical RCU critical. > > I was planning on adding a classical RCU read side critical section > whenever we called the lookup functions. > > Would that have worked? (for the sake of learning). ah. ic. You meant local_storage could be under rcu_read_lock() if we wanted to since it is not exposed to the sleepable bpf_prog which is under rcu_read_lock_trace()? It should work after a quick thought but then we need to figure out the needed places to add an extra rcu_read_lock(). What is the reason for doing this?
