Hi, On 12/15/2023 11:23 AM, Alexei Starovoitov wrote: > On Thu, Dec 14, 2023 at 11:15 AM John Fastabend > <john.fastabend@xxxxxxxxx> wrote: >> Alexei Starovoitov wrote: >>> On Wed, Dec 13, 2023 at 11:31 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: >>>> Hi, >>>> >>>> On 12/14/2023 2:22 PM, John Fastabend wrote: >>>>> Hou Tao wrote: >>>>>> From: Hou Tao <houtao1@xxxxxxxxxx> >>>>>> >>>>>> There is no rcu-read-lock requirement for ops->map_fd_get_ptr() or >>>>>> ops->map_fd_put_ptr(), so doesn't use rcu-read-lock for these two >>>>>> callbacks. >>>>>> >>>>>> For bpf_fd_array_map_update_elem(), accessing array->ptrs doesn't need >>>>>> rcu-read-lock because array->ptrs must still be allocated. For >>>>>> bpf_fd_htab_map_update_elem(), htab_map_update_elem() only requires >>>>>> rcu-read-lock to be held to avoid the WARN_ON_ONCE(), so only use >>>>>> rcu_read_lock() during the invocation of htab_map_update_elem(). >>>>>> >>>>>> Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx> >>>>>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx> >>>>>> --- >>>>>> kernel/bpf/hashtab.c | 6 ++++++ >>>>>> kernel/bpf/syscall.c | 4 ---- >>>>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c >>>>>> index 5b9146fa825f..ec3bdcc6a3cf 100644 >>>>>> --- a/kernel/bpf/hashtab.c >>>>>> +++ b/kernel/bpf/hashtab.c >>>>>> @@ -2523,7 +2523,13 @@ int bpf_fd_htab_map_update_elem(struct bpf_map *map, struct file *map_file, >>>>>> if (IS_ERR(ptr)) >>>>>> return PTR_ERR(ptr); >>>>>> >>>>>> + /* The htab bucket lock is always held during update operations in fd >>>>>> + * htab map, and the following rcu_read_lock() is only used to avoid >>>>>> + * the WARN_ON_ONCE in htab_map_update_elem(). >>>>>> + */ >> Ah ok but isn't this comment wrong because you do need rcu read lock to do >> the walk with lookup_nulls_elem_raw where there is no lock being held? And >> then the subsequent copy in place is fine because you do have a lock. > Ohh. You're correct. > Not sure what I was thinking. > > Hou, > could you please send a follow up to undo my braino. Er, I didn't follow. There is no spin-lock support in fd htab map, so htab_map_update_elem() won't call lookup_nulls_elem_raw(), instead it will lock the bucket and invoke lookup_elem_raw(), so I don't think rcu_read_lock() is indeed needed for the invocation of htab_map_update_elem(), except to make WARN_ON_ONC() happy.
