Toke Høiland-Jørgensen wrote: > Hangbin Liu <liuhangbin@xxxxxxxxx> writes: > > > Hi John, > > > > Thanks for the reviewing. > > > > On Sun, Jan 17, 2021 at 04:10:40PM -0800, John Fastabend wrote: > >> > + * The forwarding *map* could be either BPF_MAP_TYPE_DEVMAP or > >> > + * BPF_MAP_TYPE_DEVMAP_HASH. But the *ex_map* must be > >> > + * BPF_MAP_TYPE_DEVMAP_HASH to get better performance. > >> > >> Would be good to add a note ex_map _must_ be keyed by ifindex for the > >> helper to work. Its the obvious way to key a hashmap, but not required > >> iirc. > > > > OK, I will. [...] > >> WRITE_ONCE(ri->ex_map)? > >> > >> > WRITE_ONCE(ri->map, NULL); > >> > >> So we needed write_once, read_once pairs for ri->map do we also need them in > >> the ex_map case? > > > > Toke said this is no need for this read/write_once as there is already one. > > > > https://lore.kernel.org/bpf/87r1wd2bqu.fsf@xxxxxxx/ > > And then I corrected that after I figured out the real reason :) > > https://lore.kernel.org/bpf/878si2h3sb.fsf@xxxxxxx/ - Quote: > > > The READ_ONCE() is not needed because the ex_map field is only ever read > > from or written to by the CPU owning the per-cpu pointer. Whereas the > > 'map' field is manipulated by remote CPUs in bpf_clear_redirect_map(). > > So you need neither READ_ONCE() nor WRITE_ONCE() on ex_map, just like > > there are none on tgt_index and tgt_value. > > -Toke > Hi Hangbin, please add a comment above that code block to remind us why the READ_ONCE/WRITE_ONCE is not needed or add it in the commit message so we don't lose it. It seems we've hashed it over already, but I forgot after the holidays/break so presumably I'll forget next time I read this code as well and commit-msg or comment will help. Thanks, John
