On Thu, Dec 5, 2024 at 4:48 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > > Hi, > > On 12/6/2024 1:06 AM, Alexei Starovoitov wrote: > > On Thu, Dec 5, 2024 at 12:53 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > >> Hi, > >> > >> On 12/3/2024 9:42 AM, Alexei Starovoitov wrote: > >>> On Fri, Nov 29, 2024 at 4:18 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > >>>> Hou Tao <houtao@xxxxxxxxxxxxxxx> writes: > >>>> > >>>>> From: Hou Tao <houtao1@xxxxxxxxxx> > >>>>> > >>>>> After switching from kmalloc() to the bpf memory allocator, there will be > >>>>> no blocking operation during the update of LPM trie. Therefore, change > >>>>> trie->lock from spinlock_t to raw_spinlock_t to make LPM trie usable in > >>>>> atomic context, even on RT kernels. > >>>>> > >>>>> The max value of prefixlen is 2048. Therefore, update or deletion > >>>>> operations will find the target after at most 2048 comparisons. > >>>>> Constructing a test case which updates an element after 2048 comparisons > >>>>> under a 8 CPU VM, and the average time and the maximal time for such > >>>>> update operation is about 210us and 900us. > >>>> That is... quite a long time? I'm not sure we have any guidance on what > >>>> the maximum acceptable time is (perhaps the RT folks can weigh in > >>>> here?), but stalling for almost a millisecond seems long. > >>>> > >>>> Especially doing this unconditionally seems a bit risky; this means that > >>>> even a networking program using the lpm map in the data path can stall > >>>> the system for that long, even if it would have been perfectly happy to > >>>> be preempted. > >>> I don't share this concern. > >>> 2048 comparisons is an extreme case. > >>> I'm sure there are a million other ways to stall bpf prog for that long. > >> 2048 is indeed an extreme case. I would do some test to check how much > >> time is used for the normal cases with prefixlen=32 or prefixlen=128. > > Before you do that please respin with comments addressed, so we can > > land the fixes asap. > > OK. Original I thought there was no need for respin. Before posting the > v3, I want to confirm the comments which need to be addressed in the new > revision: > > 1) [PATCH bpf v2 6/9] bpf: Switch to bpf mem allocator for LPM trie > Move bpf_mem_cache_free_rcu outside of the locked scope (From Alexei) > Move the first lpm_trie_node_alloc() outside of the locked scope (There > will be no refill under irq disabled region) > > 2) [PATCH bpf v2 2/9] bpf: Remove unnecessary kfree(im_node) in > lpm_trie_update_elem > Remove the NULL init of im_node (From Daniel) Looks about right. That was 9 days ago. I cannot keep ctx for so long. Re-read the threads just in case. If you miss something it's not a big deal either.
