Hi,
On 6/28/2023 9:51 AM, Alexei Starovoitov wrote:
> On Tue, Jun 27, 2023 at 6:43 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>>
SNIP
>>> re: draining.
>>> I'll switch to do if (draing) free_all; else call_rcu; scheme
>>> to address potential memory leak though I wasn't able to repro it.
>> For v2, it was also hard for me to reproduce the leak problem. But after
>> I injected some delay by using udelay() in __free_by_rcu/__free_rcu()
>> after reading c->draining, it was relatively easy to reproduce the problems.
> 1. Please respin htab bench.
> We're still discussing patching without having the same base line.
Almost done. Need to do benchmark again to update the numbers in commit
message.
>
> 2. 'adding udelay()' is too vague. Pls post a diff hunk of what exactly
> you mean.
--- a/kernel/bpf/memalloc.c
+++ b/kernel/bpf/memalloc.c
@@ -4,6 +4,7 @@
#include <linux/llist.h>
#include <linux/bpf.h>
#include <linux/irq_work.h>
+#include <linux/delay.h>
#include <linux/bpf_mem_alloc.h>
#include <linux/memcontrol.h>
#include <asm/local.h>
@@ -261,12 +262,17 @@ static int free_all(struct llist_node *llnode,
bool percpu)
return cnt;
}
+static unsigned int delay;
+module_param(delay, uint, 0644);
+
static void __free_rcu(struct rcu_head *head)
{
struct bpf_mem_cache *c = container_of(head, struct
bpf_mem_cache, rcu_ttrace);
if (unlikely(READ_ONCE(c->draining)))
goto out;
+ if (delay)
+ udelay(delay);
free_all(llist_del_all(&c->waiting_for_gp_ttrace),
!!c->percpu_size);
out:
atomic_set(&c->call_rcu_ttrace_in_progress, 0);
@@ -361,6 +367,8 @@ static void __free_by_rcu(struct rcu_head *head)
if (unlikely(READ_ONCE(c->draining)))
goto out;
+ if (delay)
+ udelay(delay);
llnode = llist_del_all(&c->waiting_for_gp);
if (!llnode)
>
> 3. I'll send v3 shortly. Let's move discussion there.
Sure.
