* [PATCH 1/1] kasan: fix livelock in qlist_move_cache
@ 2017-11-28 4:04 zhouzhouyi
2017-11-28 4:05 ` Zhouyi Zhou
0 siblings, 1 reply; 15+ messages in thread
From: zhouzhouyi @ 2017-11-28 4:04 UTC (permalink / raw)
To: aryabinin, glider, dvyukov, kasan-dev, linux-mm, linux-kernel; +Cc: Zhouyi Zhou
From: Zhouyi Zhou <zhouzhouyi@gmail.com>
This patch fix livelock by conditionally release cpu to let others
has a chance to run.
Tested on x86_64.
Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
---
mm/kasan/quarantine.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 3a8ddf8..33eeff4 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
struct kmem_cache *cache)
{
struct qlist_node *curr;
+ struct qlist_head tmp_head;
+ unsigned long flags;
if (unlikely(qlist_empty(from)))
return;
+ qlist_init(&tmp_head);
curr = from->head;
qlist_init(from);
while (curr) {
@@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
if (obj_cache == cache)
qlist_put(to, curr, obj_cache->size);
else
- qlist_put(from, curr, obj_cache->size);
+ qlist_put(&tmp_head, curr, obj_cache->size);
curr = next;
+
+ if (need_resched()) {
+ spin_unlock_irqrestore(&quarantine_lock, flags);
+ cond_resched();
+ spin_lock_irqsave(&quarantine_lock, flags);
+ }
}
+ qlist_move_all(&tmp_head, from);
}
static void per_cpu_remove_cache(void *arg)
--
2.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 4:04 [PATCH 1/1] kasan: fix livelock in qlist_move_cache zhouzhouyi
@ 2017-11-28 4:05 ` Zhouyi Zhou
2017-11-28 7:45 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Zhouyi Zhou @ 2017-11-28 4:05 UTC (permalink / raw)
To: aryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
linux-mm, linux-kernel
Cc: Zhouyi Zhou
When there are huge amount of quarantined cache allocates in system,
number of entries in global_quarantine[i] will be great. Meanwhile,
there is no relax in while loop in function qlist_move_cache which
hold quarantine_lock. As a result, some userspace programs for example
libvirt will complain.
On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>
> This patch fix livelock by conditionally release cpu to let others
> has a chance to run.
>
> Tested on x86_64.
> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> ---
> mm/kasan/quarantine.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 3a8ddf8..33eeff4 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
> struct kmem_cache *cache)
> {
> struct qlist_node *curr;
> + struct qlist_head tmp_head;
> + unsigned long flags;
>
> if (unlikely(qlist_empty(from)))
> return;
>
> + qlist_init(&tmp_head);
> curr = from->head;
> qlist_init(from);
> while (curr) {
> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
> if (obj_cache == cache)
> qlist_put(to, curr, obj_cache->size);
> else
> - qlist_put(from, curr, obj_cache->size);
> + qlist_put(&tmp_head, curr, obj_cache->size);
>
> curr = next;
> +
> + if (need_resched()) {
> + spin_unlock_irqrestore(&quarantine_lock, flags);
> + cond_resched();
> + spin_lock_irqsave(&quarantine_lock, flags);
> + }
> }
> + qlist_move_all(&tmp_head, from);
> }
>
> static void per_cpu_remove_cache(void *arg)
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 4:05 ` Zhouyi Zhou
@ 2017-11-28 7:45 ` Dmitry Vyukov
2017-11-28 8:00 ` Zhouyi Zhou
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-11-28 7:45 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> When there are huge amount of quarantined cache allocates in system,
> number of entries in global_quarantine[i] will be great. Meanwhile,
> there is no relax in while loop in function qlist_move_cache which
> hold quarantine_lock. As a result, some userspace programs for example
> libvirt will complain.
Hi,
The QUARANTINE_BATCHES thing was supposed to fix this problem, see
quarantine_remove_cache() function.
What is the amount of RAM and number of CPUs in your system?
If system has 4GB of RAM, quarantine size is 128MB and that's split
into 1024 batches. Batch size is 128KB. Even if that's filled with the
smallest objects of size 32, that's only 4K objects. And there is a
cond_resched() between processing of every batch.
I don't understand why it causes problems in your setup. We use KASAN
extremely heavily on hundreds of machines 24x7 and we have not seen
any single report from this code...
> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>
>> This patch fix livelock by conditionally release cpu to let others
>> has a chance to run.
>>
>> Tested on x86_64.
>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>> ---
>> mm/kasan/quarantine.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>> index 3a8ddf8..33eeff4 100644
>> --- a/mm/kasan/quarantine.c
>> +++ b/mm/kasan/quarantine.c
>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>> struct kmem_cache *cache)
>> {
>> struct qlist_node *curr;
>> + struct qlist_head tmp_head;
>> + unsigned long flags;
>>
>> if (unlikely(qlist_empty(from)))
>> return;
>>
>> + qlist_init(&tmp_head);
>> curr = from->head;
>> qlist_init(from);
>> while (curr) {
>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>> if (obj_cache == cache)
>> qlist_put(to, curr, obj_cache->size);
>> else
>> - qlist_put(from, curr, obj_cache->size);
>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>
>> curr = next;
>> +
>> + if (need_resched()) {
>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>> + cond_resched();
>> + spin_lock_irqsave(&quarantine_lock, flags);
>> + }
>> }
>> + qlist_move_all(&tmp_head, from);
>> }
>>
>> static void per_cpu_remove_cache(void *arg)
>> --
>> 2.1.4
>>
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 7:45 ` Dmitry Vyukov
@ 2017-11-28 8:00 ` Zhouyi Zhou
2017-11-28 8:12 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Zhouyi Zhou @ 2017-11-28 8:00 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
Thanks for reviewing
My machine has 128G of RAM, and runs many KVM virtual machines.
libvirtd always
report "internal error: received hangup / error event on socket" under
heavy memory load.
Then I use perf top -g, qlist_move_cache consumes 100% cpu for
several minutes.
On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>> When there are huge amount of quarantined cache allocates in system,
>> number of entries in global_quarantine[i] will be great. Meanwhile,
>> there is no relax in while loop in function qlist_move_cache which
>> hold quarantine_lock. As a result, some userspace programs for example
>> libvirt will complain.
>
> Hi,
>
> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
> quarantine_remove_cache() function.
> What is the amount of RAM and number of CPUs in your system?
> If system has 4GB of RAM, quarantine size is 128MB and that's split
> into 1024 batches. Batch size is 128KB. Even if that's filled with the
> smallest objects of size 32, that's only 4K objects. And there is a
> cond_resched() between processing of every batch.
> I don't understand why it causes problems in your setup. We use KASAN
> extremely heavily on hundreds of machines 24x7 and we have not seen
> any single report from this code...
>
>
>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>
>>> This patch fix livelock by conditionally release cpu to let others
>>> has a chance to run.
>>>
>>> Tested on x86_64.
>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>> ---
>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>> index 3a8ddf8..33eeff4 100644
>>> --- a/mm/kasan/quarantine.c
>>> +++ b/mm/kasan/quarantine.c
>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>> struct kmem_cache *cache)
>>> {
>>> struct qlist_node *curr;
>>> + struct qlist_head tmp_head;
>>> + unsigned long flags;
>>>
>>> if (unlikely(qlist_empty(from)))
>>> return;
>>>
>>> + qlist_init(&tmp_head);
>>> curr = from->head;
>>> qlist_init(from);
>>> while (curr) {
>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>> if (obj_cache == cache)
>>> qlist_put(to, curr, obj_cache->size);
>>> else
>>> - qlist_put(from, curr, obj_cache->size);
>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>
>>> curr = next;
>>> +
>>> + if (need_resched()) {
>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>> + cond_resched();
>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>> + }
>>> }
>>> + qlist_move_all(&tmp_head, from);
>>> }
>>>
>>> static void per_cpu_remove_cache(void *arg)
>>> --
>>> 2.1.4
>>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>> To post to this group, send email to kasan-dev@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 8:00 ` Zhouyi Zhou
@ 2017-11-28 8:12 ` Dmitry Vyukov
2017-11-28 8:33 ` Zhouyi Zhou
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-11-28 8:12 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
On Tue, Nov 28, 2017 at 9:00 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Thanks for reviewing
> My machine has 128G of RAM, and runs many KVM virtual machines.
> libvirtd always
> report "internal error: received hangup / error event on socket" under
> heavy memory load.
> Then I use perf top -g, qlist_move_cache consumes 100% cpu for
> several minutes.
For 128GB of RAM, batch size is 4MB. Processing such batch should not
take more than few ms. So I am still struggling to understand how/why
your change helps and why there are issues in the first place...
> On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>> When there are huge amount of quarantined cache allocates in system,
>>> number of entries in global_quarantine[i] will be great. Meanwhile,
>>> there is no relax in while loop in function qlist_move_cache which
>>> hold quarantine_lock. As a result, some userspace programs for example
>>> libvirt will complain.
>>
>> Hi,
>>
>> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
>> quarantine_remove_cache() function.
>> What is the amount of RAM and number of CPUs in your system?
>> If system has 4GB of RAM, quarantine size is 128MB and that's split
>> into 1024 batches. Batch size is 128KB. Even if that's filled with the
>> smallest objects of size 32, that's only 4K objects. And there is a
>> cond_resched() between processing of every batch.
>> I don't understand why it causes problems in your setup. We use KASAN
>> extremely heavily on hundreds of machines 24x7 and we have not seen
>> any single report from this code...
>>
>>
>>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>
>>>> This patch fix livelock by conditionally release cpu to let others
>>>> has a chance to run.
>>>>
>>>> Tested on x86_64.
>>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>> ---
>>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>>> index 3a8ddf8..33eeff4 100644
>>>> --- a/mm/kasan/quarantine.c
>>>> +++ b/mm/kasan/quarantine.c
>>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>>> struct kmem_cache *cache)
>>>> {
>>>> struct qlist_node *curr;
>>>> + struct qlist_head tmp_head;
>>>> + unsigned long flags;
>>>>
>>>> if (unlikely(qlist_empty(from)))
>>>> return;
>>>>
>>>> + qlist_init(&tmp_head);
>>>> curr = from->head;
>>>> qlist_init(from);
>>>> while (curr) {
>>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>>> if (obj_cache == cache)
>>>> qlist_put(to, curr, obj_cache->size);
>>>> else
>>>> - qlist_put(from, curr, obj_cache->size);
>>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>>
>>>> curr = next;
>>>> +
>>>> + if (need_resched()) {
>>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>>> + cond_resched();
>>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>>> + }
>>>> }
>>>> + qlist_move_all(&tmp_head, from);
>>>> }
>>>>
>>>> static void per_cpu_remove_cache(void *arg)
>>>> --
>>>> 2.1.4
>>>>
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>>> To post to this group, send email to kasan-dev@googlegroups.com.
>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 8:12 ` Dmitry Vyukov
@ 2017-11-28 8:33 ` Zhouyi Zhou
2017-11-28 8:58 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Zhouyi Zhou @ 2017-11-28 8:33 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
Hi,
Please take a look at function quarantine_put, I don't think following
code will limit the batch size below quarantine_batch_size. It only advance
quarantine_tail after qlist_move_all.
qlist_move_all(q, &temp);
spin_lock(&quarantine_lock);
WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
if (global_quarantine[quarantine_tail].bytes >=
READ_ONCE(quarantine_batch_size)) {
int new_tail;
new_tail = quarantine_tail + 1;
if (new_tail == QUARANTINE_BATCHES)
new_tail = 0;
if (new_tail != quarantine_head)
quarantine_tail = new_tail;
On Tue, Nov 28, 2017 at 4:12 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 28, 2017 at 9:00 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>> Thanks for reviewing
>> My machine has 128G of RAM, and runs many KVM virtual machines.
>> libvirtd always
>> report "internal error: received hangup / error event on socket" under
>> heavy memory load.
>> Then I use perf top -g, qlist_move_cache consumes 100% cpu for
>> several minutes.
>
> For 128GB of RAM, batch size is 4MB. Processing such batch should not
> take more than few ms. So I am still struggling to understand how/why
> your change helps and why there are issues in the first place...
>
>
>
>> On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>> When there are huge amount of quarantined cache allocates in system,
>>>> number of entries in global_quarantine[i] will be great. Meanwhile,
>>>> there is no relax in while loop in function qlist_move_cache which
>>>> hold quarantine_lock. As a result, some userspace programs for example
>>>> libvirt will complain.
>>>
>>> Hi,
>>>
>>> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
>>> quarantine_remove_cache() function.
>>> What is the amount of RAM and number of CPUs in your system?
>>> If system has 4GB of RAM, quarantine size is 128MB and that's split
>>> into 1024 batches. Batch size is 128KB. Even if that's filled with the
>>> smallest objects of size 32, that's only 4K objects. And there is a
>>> cond_resched() between processing of every batch.
>>> I don't understand why it causes problems in your setup. We use KASAN
>>> extremely heavily on hundreds of machines 24x7 and we have not seen
>>> any single report from this code...
>>>
>>>
>>>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>
>>>>> This patch fix livelock by conditionally release cpu to let others
>>>>> has a chance to run.
>>>>>
>>>>> Tested on x86_64.
>>>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>> ---
>>>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>>>> index 3a8ddf8..33eeff4 100644
>>>>> --- a/mm/kasan/quarantine.c
>>>>> +++ b/mm/kasan/quarantine.c
>>>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>> struct kmem_cache *cache)
>>>>> {
>>>>> struct qlist_node *curr;
>>>>> + struct qlist_head tmp_head;
>>>>> + unsigned long flags;
>>>>>
>>>>> if (unlikely(qlist_empty(from)))
>>>>> return;
>>>>>
>>>>> + qlist_init(&tmp_head);
>>>>> curr = from->head;
>>>>> qlist_init(from);
>>>>> while (curr) {
>>>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>> if (obj_cache == cache)
>>>>> qlist_put(to, curr, obj_cache->size);
>>>>> else
>>>>> - qlist_put(from, curr, obj_cache->size);
>>>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>>>
>>>>> curr = next;
>>>>> +
>>>>> + if (need_resched()) {
>>>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>>>> + cond_resched();
>>>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>>>> + }
>>>>> }
>>>>> + qlist_move_all(&tmp_head, from);
>>>>> }
>>>>>
>>>>> static void per_cpu_remove_cache(void *arg)
>>>>> --
>>>>> 2.1.4
>>>>>
>>>>
>>>> --
>>>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>>>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>>>> To post to this group, send email to kasan-dev@googlegroups.com.
>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>>>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 8:33 ` Zhouyi Zhou
@ 2017-11-28 8:58 ` Dmitry Vyukov
2017-11-28 9:17 ` Zhouyi Zhou
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-11-28 8:58 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
On Tue, Nov 28, 2017 at 9:33 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Hi,
> Please take a look at function quarantine_put, I don't think following
> code will limit the batch size below quarantine_batch_size. It only advance
> quarantine_tail after qlist_move_all.
>
> qlist_move_all(q, &temp);
>
> spin_lock(&quarantine_lock);
> WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
> qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
> if (global_quarantine[quarantine_tail].bytes >=
> READ_ONCE(quarantine_batch_size)) {
> int new_tail;
>
> new_tail = quarantine_tail + 1;
> if (new_tail == QUARANTINE_BATCHES)
> new_tail = 0;
> if (new_tail != quarantine_head)
> quarantine_tail = new_tail;
As far as I see this code can exceed global quarantine batch size by
at most 1 per-cpu batch. Per-cpu batch is caped at 1MB. So max global
batch size will be 4MB+1MB. Which does not radically change situation.
> On Tue, Nov 28, 2017 at 4:12 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Nov 28, 2017 at 9:00 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>> Thanks for reviewing
>>> My machine has 128G of RAM, and runs many KVM virtual machines.
>>> libvirtd always
>>> report "internal error: received hangup / error event on socket" under
>>> heavy memory load.
>>> Then I use perf top -g, qlist_move_cache consumes 100% cpu for
>>> several minutes.
>>
>> For 128GB of RAM, batch size is 4MB. Processing such batch should not
>> take more than few ms. So I am still struggling to understand how/why
>> your change helps and why there are issues in the first place...
>>
>>
>>
>>> On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>> When there are huge amount of quarantined cache allocates in system,
>>>>> number of entries in global_quarantine[i] will be great. Meanwhile,
>>>>> there is no relax in while loop in function qlist_move_cache which
>>>>> hold quarantine_lock. As a result, some userspace programs for example
>>>>> libvirt will complain.
>>>>
>>>> Hi,
>>>>
>>>> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
>>>> quarantine_remove_cache() function.
>>>> What is the amount of RAM and number of CPUs in your system?
>>>> If system has 4GB of RAM, quarantine size is 128MB and that's split
>>>> into 1024 batches. Batch size is 128KB. Even if that's filled with the
>>>> smallest objects of size 32, that's only 4K objects. And there is a
>>>> cond_resched() between processing of every batch.
>>>> I don't understand why it causes problems in your setup. We use KASAN
>>>> extremely heavily on hundreds of machines 24x7 and we have not seen
>>>> any single report from this code...
>>>>
>>>>
>>>>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>>>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>
>>>>>> This patch fix livelock by conditionally release cpu to let others
>>>>>> has a chance to run.
>>>>>>
>>>>>> Tested on x86_64.
>>>>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>> ---
>>>>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>>>>> index 3a8ddf8..33eeff4 100644
>>>>>> --- a/mm/kasan/quarantine.c
>>>>>> +++ b/mm/kasan/quarantine.c
>>>>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>> struct kmem_cache *cache)
>>>>>> {
>>>>>> struct qlist_node *curr;
>>>>>> + struct qlist_head tmp_head;
>>>>>> + unsigned long flags;
>>>>>>
>>>>>> if (unlikely(qlist_empty(from)))
>>>>>> return;
>>>>>>
>>>>>> + qlist_init(&tmp_head);
>>>>>> curr = from->head;
>>>>>> qlist_init(from);
>>>>>> while (curr) {
>>>>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>> if (obj_cache == cache)
>>>>>> qlist_put(to, curr, obj_cache->size);
>>>>>> else
>>>>>> - qlist_put(from, curr, obj_cache->size);
>>>>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>>>>
>>>>>> curr = next;
>>>>>> +
>>>>>> + if (need_resched()) {
>>>>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>>>>> + cond_resched();
>>>>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>>>>> + }
>>>>>> }
>>>>>> + qlist_move_all(&tmp_head, from);
>>>>>> }
>>>>>>
>>>>>> static void per_cpu_remove_cache(void *arg)
>>>>>> --
>>>>>> 2.1.4
>>>>>>
>>>>>
>>>>> --
>>>>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>>>>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>>>>> To post to this group, send email to kasan-dev@googlegroups.com.
>>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>>>>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 8:58 ` Dmitry Vyukov
@ 2017-11-28 9:17 ` Zhouyi Zhou
2017-11-28 9:27 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Zhouyi Zhou @ 2017-11-28 9:17 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
Hi,
Imagine all of the QUARANTINE_BATCHES elements of
global_quarantine array is of size 4MB + 1MB, now a new call
to quarantine_put is invoked, one of the element will be of size 4MB +
1MB + 1MB, so on and on.
On Tue, Nov 28, 2017 at 4:58 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 28, 2017 at 9:33 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>> Hi,
>> Please take a look at function quarantine_put, I don't think following
>> code will limit the batch size below quarantine_batch_size. It only advance
>> quarantine_tail after qlist_move_all.
>>
>> qlist_move_all(q, &temp);
>>
>> spin_lock(&quarantine_lock);
>> WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
>> qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
>> if (global_quarantine[quarantine_tail].bytes >=
>> READ_ONCE(quarantine_batch_size)) {
>> int new_tail;
>>
>> new_tail = quarantine_tail + 1;
>> if (new_tail == QUARANTINE_BATCHES)
>> new_tail = 0;
>> if (new_tail != quarantine_head)
>> quarantine_tail = new_tail;
>
>
> As far as I see this code can exceed global quarantine batch size by
> at most 1 per-cpu batch. Per-cpu batch is caped at 1MB. So max global
> batch size will be 4MB+1MB. Which does not radically change situation.
>
>
>> On Tue, Nov 28, 2017 at 4:12 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> On Tue, Nov 28, 2017 at 9:00 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>> Thanks for reviewing
>>>> My machine has 128G of RAM, and runs many KVM virtual machines.
>>>> libvirtd always
>>>> report "internal error: received hangup / error event on socket" under
>>>> heavy memory load.
>>>> Then I use perf top -g, qlist_move_cache consumes 100% cpu for
>>>> several minutes.
>>>
>>> For 128GB of RAM, batch size is 4MB. Processing such batch should not
>>> take more than few ms. So I am still struggling to understand how/why
>>> your change helps and why there are issues in the first place...
>>>
>>>
>>>
>>>> On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>>> When there are huge amount of quarantined cache allocates in system,
>>>>>> number of entries in global_quarantine[i] will be great. Meanwhile,
>>>>>> there is no relax in while loop in function qlist_move_cache which
>>>>>> hold quarantine_lock. As a result, some userspace programs for example
>>>>>> libvirt will complain.
>>>>>
>>>>> Hi,
>>>>>
>>>>> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
>>>>> quarantine_remove_cache() function.
>>>>> What is the amount of RAM and number of CPUs in your system?
>>>>> If system has 4GB of RAM, quarantine size is 128MB and that's split
>>>>> into 1024 batches. Batch size is 128KB. Even if that's filled with the
>>>>> smallest objects of size 32, that's only 4K objects. And there is a
>>>>> cond_resched() between processing of every batch.
>>>>> I don't understand why it causes problems in your setup. We use KASAN
>>>>> extremely heavily on hundreds of machines 24x7 and we have not seen
>>>>> any single report from this code...
>>>>>
>>>>>
>>>>>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>>>>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>>
>>>>>>> This patch fix livelock by conditionally release cpu to let others
>>>>>>> has a chance to run.
>>>>>>>
>>>>>>> Tested on x86_64.
>>>>>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>> ---
>>>>>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>>>>>> index 3a8ddf8..33eeff4 100644
>>>>>>> --- a/mm/kasan/quarantine.c
>>>>>>> +++ b/mm/kasan/quarantine.c
>>>>>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>> struct kmem_cache *cache)
>>>>>>> {
>>>>>>> struct qlist_node *curr;
>>>>>>> + struct qlist_head tmp_head;
>>>>>>> + unsigned long flags;
>>>>>>>
>>>>>>> if (unlikely(qlist_empty(from)))
>>>>>>> return;
>>>>>>>
>>>>>>> + qlist_init(&tmp_head);
>>>>>>> curr = from->head;
>>>>>>> qlist_init(from);
>>>>>>> while (curr) {
>>>>>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>> if (obj_cache == cache)
>>>>>>> qlist_put(to, curr, obj_cache->size);
>>>>>>> else
>>>>>>> - qlist_put(from, curr, obj_cache->size);
>>>>>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>>>>>
>>>>>>> curr = next;
>>>>>>> +
>>>>>>> + if (need_resched()) {
>>>>>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>>>>>> + cond_resched();
>>>>>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>>>>>> + }
>>>>>>> }
>>>>>>> + qlist_move_all(&tmp_head, from);
>>>>>>> }
>>>>>>>
>>>>>>> static void per_cpu_remove_cache(void *arg)
>>>>>>> --
>>>>>>> 2.1.4
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>>>>>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>>>>>> To post to this group, send email to kasan-dev@googlegroups.com.
>>>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>>>>>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 9:17 ` Zhouyi Zhou
@ 2017-11-28 9:27 ` Dmitry Vyukov
2017-11-28 11:30 ` Zhouyi Zhou
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-11-28 9:27 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
On Tue, Nov 28, 2017 at 10:17 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Hi,
> Imagine all of the QUARANTINE_BATCHES elements of
> global_quarantine array is of size 4MB + 1MB, now a new call
> to quarantine_put is invoked, one of the element will be of size 4MB +
> 1MB + 1MB, so on and on.
I see what you mean. Does it really happen in your case? What's the
maximum batch size that you get during your workload?
I always wondered why don't we drain quarantine right in
quarantine_put when we overflow it? We already take quarantine_lock
and calling cache_free should be fine in that context, since user code
already does that.
> On Tue, Nov 28, 2017 at 4:58 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Nov 28, 2017 at 9:33 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>> Hi,
>>> Please take a look at function quarantine_put, I don't think following
>>> code will limit the batch size below quarantine_batch_size. It only advance
>>> quarantine_tail after qlist_move_all.
>>>
>>> qlist_move_all(q, &temp);
>>>
>>> spin_lock(&quarantine_lock);
>>> WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
>>> qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
>>> if (global_quarantine[quarantine_tail].bytes >=
>>> READ_ONCE(quarantine_batch_size)) {
>>> int new_tail;
>>>
>>> new_tail = quarantine_tail + 1;
>>> if (new_tail == QUARANTINE_BATCHES)
>>> new_tail = 0;
>>> if (new_tail != quarantine_head)
>>> quarantine_tail = new_tail;
>>
>>
>> As far as I see this code can exceed global quarantine batch size by
>> at most 1 per-cpu batch. Per-cpu batch is caped at 1MB. So max global
>> batch size will be 4MB+1MB. Which does not radically change situation.
>>
>>
>>> On Tue, Nov 28, 2017 at 4:12 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> On Tue, Nov 28, 2017 at 9:00 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>> Thanks for reviewing
>>>>> My machine has 128G of RAM, and runs many KVM virtual machines.
>>>>> libvirtd always
>>>>> report "internal error: received hangup / error event on socket" under
>>>>> heavy memory load.
>>>>> Then I use perf top -g, qlist_move_cache consumes 100% cpu for
>>>>> several minutes.
>>>>
>>>> For 128GB of RAM, batch size is 4MB. Processing such batch should not
>>>> take more than few ms. So I am still struggling to understand how/why
>>>> your change helps and why there are issues in the first place...
>>>>
>>>>
>>>>
>>>>> On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>>> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>>>> When there are huge amount of quarantined cache allocates in system,
>>>>>>> number of entries in global_quarantine[i] will be great. Meanwhile,
>>>>>>> there is no relax in while loop in function qlist_move_cache which
>>>>>>> hold quarantine_lock. As a result, some userspace programs for example
>>>>>>> libvirt will complain.
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
>>>>>> quarantine_remove_cache() function.
>>>>>> What is the amount of RAM and number of CPUs in your system?
>>>>>> If system has 4GB of RAM, quarantine size is 128MB and that's split
>>>>>> into 1024 batches. Batch size is 128KB. Even if that's filled with the
>>>>>> smallest objects of size 32, that's only 4K objects. And there is a
>>>>>> cond_resched() between processing of every batch.
>>>>>> I don't understand why it causes problems in your setup. We use KASAN
>>>>>> extremely heavily on hundreds of machines 24x7 and we have not seen
>>>>>> any single report from this code...
>>>>>>
>>>>>>
>>>>>>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>>>>>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>>>
>>>>>>>> This patch fix livelock by conditionally release cpu to let others
>>>>>>>> has a chance to run.
>>>>>>>>
>>>>>>>> Tested on x86_64.
>>>>>>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>>> ---
>>>>>>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>>>>>>> index 3a8ddf8..33eeff4 100644
>>>>>>>> --- a/mm/kasan/quarantine.c
>>>>>>>> +++ b/mm/kasan/quarantine.c
>>>>>>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>>> struct kmem_cache *cache)
>>>>>>>> {
>>>>>>>> struct qlist_node *curr;
>>>>>>>> + struct qlist_head tmp_head;
>>>>>>>> + unsigned long flags;
>>>>>>>>
>>>>>>>> if (unlikely(qlist_empty(from)))
>>>>>>>> return;
>>>>>>>>
>>>>>>>> + qlist_init(&tmp_head);
>>>>>>>> curr = from->head;
>>>>>>>> qlist_init(from);
>>>>>>>> while (curr) {
>>>>>>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>>> if (obj_cache == cache)
>>>>>>>> qlist_put(to, curr, obj_cache->size);
>>>>>>>> else
>>>>>>>> - qlist_put(from, curr, obj_cache->size);
>>>>>>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>>>>>>
>>>>>>>> curr = next;
>>>>>>>> +
>>>>>>>> + if (need_resched()) {
>>>>>>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>>>>>>> + cond_resched();
>>>>>>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>>>>>>> + }
>>>>>>>> }
>>>>>>>> + qlist_move_all(&tmp_head, from);
>>>>>>>> }
>>>>>>>>
>>>>>>>> static void per_cpu_remove_cache(void *arg)
>>>>>>>> --
>>>>>>>> 2.1.4
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>>>>>>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>>>>>>> To post to this group, send email to kasan-dev@googlegroups.com.
>>>>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>>>>>>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 9:27 ` Dmitry Vyukov
@ 2017-11-28 11:30 ` Zhouyi Zhou
2017-11-28 17:56 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Zhouyi Zhou @ 2017-11-28 11:30 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
Hi,
By using perf top, qlist_move_cache occupies 100% cpu did really
happen in my environment yesterday, or I
won't notice the kasan code.
Currently I have difficulty to let it reappear because the frontend
guy modified some user mode code.
I can repeat again and again now is
kgdb_breakpoint () at kernel/debug/debug_core.c:1073
1073 wmb(); /* Sync point after breakpoint */
(gdb) p quarantine_batch_size
$1 = 3601946
And by instrument code, maximum
global_quarantine[quarantine_tail].bytes reached is 6618208.
I do think drain quarantine right in quarantine_put is a better
place to drain because cache_free is fine in
that context. I am willing do it if you think it is convenient :-)
On Tue, Nov 28, 2017 at 5:27 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 28, 2017 at 10:17 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>> Hi,
>> Imagine all of the QUARANTINE_BATCHES elements of
>> global_quarantine array is of size 4MB + 1MB, now a new call
>> to quarantine_put is invoked, one of the element will be of size 4MB +
>> 1MB + 1MB, so on and on.
>
>
> I see what you mean. Does it really happen in your case? What's the
> maximum batch size that you get during your workload?
>
> I always wondered why don't we drain quarantine right in
> quarantine_put when we overflow it? We already take quarantine_lock
> and calling cache_free should be fine in that context, since user code
> already does that.
>
>
>
>> On Tue, Nov 28, 2017 at 4:58 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> On Tue, Nov 28, 2017 at 9:33 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>> Hi,
>>>> Please take a look at function quarantine_put, I don't think following
>>>> code will limit the batch size below quarantine_batch_size. It only advance
>>>> quarantine_tail after qlist_move_all.
>>>>
>>>> qlist_move_all(q, &temp);
>>>>
>>>> spin_lock(&quarantine_lock);
>>>> WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
>>>> qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
>>>> if (global_quarantine[quarantine_tail].bytes >=
>>>> READ_ONCE(quarantine_batch_size)) {
>>>> int new_tail;
>>>>
>>>> new_tail = quarantine_tail + 1;
>>>> if (new_tail == QUARANTINE_BATCHES)
>>>> new_tail = 0;
>>>> if (new_tail != quarantine_head)
>>>> quarantine_tail = new_tail;
>>>
>>>
>>> As far as I see this code can exceed global quarantine batch size by
>>> at most 1 per-cpu batch. Per-cpu batch is caped at 1MB. So max global
>>> batch size will be 4MB+1MB. Which does not radically change situation.
>>>
>>>
>>>> On Tue, Nov 28, 2017 at 4:12 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>> On Tue, Nov 28, 2017 at 9:00 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>>> Thanks for reviewing
>>>>>> My machine has 128G of RAM, and runs many KVM virtual machines.
>>>>>> libvirtd always
>>>>>> report "internal error: received hangup / error event on socket" under
>>>>>> heavy memory load.
>>>>>> Then I use perf top -g, qlist_move_cache consumes 100% cpu for
>>>>>> several minutes.
>>>>>
>>>>> For 128GB of RAM, batch size is 4MB. Processing such batch should not
>>>>> take more than few ms. So I am still struggling to understand how/why
>>>>> your change helps and why there are issues in the first place...
>>>>>
>>>>>
>>>>>
>>>>>> On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>>>> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>>>>> When there are huge amount of quarantined cache allocates in system,
>>>>>>>> number of entries in global_quarantine[i] will be great. Meanwhile,
>>>>>>>> there is no relax in while loop in function qlist_move_cache which
>>>>>>>> hold quarantine_lock. As a result, some userspace programs for example
>>>>>>>> libvirt will complain.
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
>>>>>>> quarantine_remove_cache() function.
>>>>>>> What is the amount of RAM and number of CPUs in your system?
>>>>>>> If system has 4GB of RAM, quarantine size is 128MB and that's split
>>>>>>> into 1024 batches. Batch size is 128KB. Even if that's filled with the
>>>>>>> smallest objects of size 32, that's only 4K objects. And there is a
>>>>>>> cond_resched() between processing of every batch.
>>>>>>> I don't understand why it causes problems in your setup. We use KASAN
>>>>>>> extremely heavily on hundreds of machines 24x7 and we have not seen
>>>>>>> any single report from this code...
>>>>>>>
>>>>>>>
>>>>>>>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>>>>>>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>>>>
>>>>>>>>> This patch fix livelock by conditionally release cpu to let others
>>>>>>>>> has a chance to run.
>>>>>>>>>
>>>>>>>>> Tested on x86_64.
>>>>>>>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>>>> ---
>>>>>>>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>>>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>>>>>>>> index 3a8ddf8..33eeff4 100644
>>>>>>>>> --- a/mm/kasan/quarantine.c
>>>>>>>>> +++ b/mm/kasan/quarantine.c
>>>>>>>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>>>> struct kmem_cache *cache)
>>>>>>>>> {
>>>>>>>>> struct qlist_node *curr;
>>>>>>>>> + struct qlist_head tmp_head;
>>>>>>>>> + unsigned long flags;
>>>>>>>>>
>>>>>>>>> if (unlikely(qlist_empty(from)))
>>>>>>>>> return;
>>>>>>>>>
>>>>>>>>> + qlist_init(&tmp_head);
>>>>>>>>> curr = from->head;
>>>>>>>>> qlist_init(from);
>>>>>>>>> while (curr) {
>>>>>>>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>>>> if (obj_cache == cache)
>>>>>>>>> qlist_put(to, curr, obj_cache->size);
>>>>>>>>> else
>>>>>>>>> - qlist_put(from, curr, obj_cache->size);
>>>>>>>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>>>>>>>
>>>>>>>>> curr = next;
>>>>>>>>> +
>>>>>>>>> + if (need_resched()) {
>>>>>>>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>>>>>>>> + cond_resched();
>>>>>>>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>>>>>>>> + }
>>>>>>>>> }
>>>>>>>>> + qlist_move_all(&tmp_head, from);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static void per_cpu_remove_cache(void *arg)
>>>>>>>>> --
>>>>>>>>> 2.1.4
>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>>>>>>>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>>>>>>>> To post to this group, send email to kasan-dev@googlegroups.com.
>>>>>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>>>>>>>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 11:30 ` Zhouyi Zhou
@ 2017-11-28 17:56 ` Dmitry Vyukov
2017-11-28 17:57 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-11-28 17:56 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
On Tue, Nov 28, 2017 at 12:30 PM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Hi,
> By using perf top, qlist_move_cache occupies 100% cpu did really
> happen in my environment yesterday, or I
> won't notice the kasan code.
> Currently I have difficulty to let it reappear because the frontend
> guy modified some user mode code.
> I can repeat again and again now is
> kgdb_breakpoint () at kernel/debug/debug_core.c:1073
> 1073 wmb(); /* Sync point after breakpoint */
> (gdb) p quarantine_batch_size
> $1 = 3601946
> And by instrument code, maximum
> global_quarantine[quarantine_tail].bytes reached is 6618208.
On second thought, size does not matter too much because there can be
large objects. Quarantine always quantize by objects, we can't part of
an object into one batch, and another part of the object into another
object. But it's not a problem, because overhead per objects is O(1).
We can push a single 4MB object and overflow target size by 4MB and
that will be fine.
Either way, 6MB is not terribly much too. Should take milliseconds to process.
> I do think drain quarantine right in quarantine_put is a better
> place to drain because cache_free is fine in
> that context. I am willing do it if you think it is convenient :-)
>
>
> On Tue, Nov 28, 2017 at 5:27 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Nov 28, 2017 at 10:17 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>> Hi,
>>> Imagine all of the QUARANTINE_BATCHES elements of
>>> global_quarantine array is of size 4MB + 1MB, now a new call
>>> to quarantine_put is invoked, one of the element will be of size 4MB +
>>> 1MB + 1MB, so on and on.
>>
>>
>> I see what you mean. Does it really happen in your case? What's the
>> maximum batch size that you get during your workload?
>>
>> I always wondered why don't we drain quarantine right in
>> quarantine_put when we overflow it? We already take quarantine_lock
>> and calling cache_free should be fine in that context, since user code
>> already does that.
>>
>>
>>
>>> On Tue, Nov 28, 2017 at 4:58 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> On Tue, Nov 28, 2017 at 9:33 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>> Hi,
>>>>> Please take a look at function quarantine_put, I don't think following
>>>>> code will limit the batch size below quarantine_batch_size. It only advance
>>>>> quarantine_tail after qlist_move_all.
>>>>>
>>>>> qlist_move_all(q, &temp);
>>>>>
>>>>> spin_lock(&quarantine_lock);
>>>>> WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
>>>>> qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
>>>>> if (global_quarantine[quarantine_tail].bytes >=
>>>>> READ_ONCE(quarantine_batch_size)) {
>>>>> int new_tail;
>>>>>
>>>>> new_tail = quarantine_tail + 1;
>>>>> if (new_tail == QUARANTINE_BATCHES)
>>>>> new_tail = 0;
>>>>> if (new_tail != quarantine_head)
>>>>> quarantine_tail = new_tail;
>>>>
>>>>
>>>> As far as I see this code can exceed global quarantine batch size by
>>>> at most 1 per-cpu batch. Per-cpu batch is caped at 1MB. So max global
>>>> batch size will be 4MB+1MB. Which does not radically change situation.
>>>>
>>>>
>>>>> On Tue, Nov 28, 2017 at 4:12 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>>> On Tue, Nov 28, 2017 at 9:00 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>>>> Thanks for reviewing
>>>>>>> My machine has 128G of RAM, and runs many KVM virtual machines.
>>>>>>> libvirtd always
>>>>>>> report "internal error: received hangup / error event on socket" under
>>>>>>> heavy memory load.
>>>>>>> Then I use perf top -g, qlist_move_cache consumes 100% cpu for
>>>>>>> several minutes.
>>>>>>
>>>>>> For 128GB of RAM, batch size is 4MB. Processing such batch should not
>>>>>> take more than few ms. So I am still struggling to understand how/why
>>>>>> your change helps and why there are issues in the first place...
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On Tue, Nov 28, 2017 at 3:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>>>>> On Tue, Nov 28, 2017 at 5:05 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>>>>>>> When there are huge amount of quarantined cache allocates in system,
>>>>>>>>> number of entries in global_quarantine[i] will be great. Meanwhile,
>>>>>>>>> there is no relax in while loop in function qlist_move_cache which
>>>>>>>>> hold quarantine_lock. As a result, some userspace programs for example
>>>>>>>>> libvirt will complain.
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> The QUARANTINE_BATCHES thing was supposed to fix this problem, see
>>>>>>>> quarantine_remove_cache() function.
>>>>>>>> What is the amount of RAM and number of CPUs in your system?
>>>>>>>> If system has 4GB of RAM, quarantine size is 128MB and that's split
>>>>>>>> into 1024 batches. Batch size is 128KB. Even if that's filled with the
>>>>>>>> smallest objects of size 32, that's only 4K objects. And there is a
>>>>>>>> cond_resched() between processing of every batch.
>>>>>>>> I don't understand why it causes problems in your setup. We use KASAN
>>>>>>>> extremely heavily on hundreds of machines 24x7 and we have not seen
>>>>>>>> any single report from this code...
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Tue, Nov 28, 2017 at 12:04 PM, <zhouzhouyi@gmail.com> wrote:
>>>>>>>>>> From: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>>>>>
>>>>>>>>>> This patch fix livelock by conditionally release cpu to let others
>>>>>>>>>> has a chance to run.
>>>>>>>>>>
>>>>>>>>>> Tested on x86_64.
>>>>>>>>>> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>> mm/kasan/quarantine.c | 12 +++++++++++-
>>>>>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>>>>>>>>>> index 3a8ddf8..33eeff4 100644
>>>>>>>>>> --- a/mm/kasan/quarantine.c
>>>>>>>>>> +++ b/mm/kasan/quarantine.c
>>>>>>>>>> @@ -265,10 +265,13 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>>>>> struct kmem_cache *cache)
>>>>>>>>>> {
>>>>>>>>>> struct qlist_node *curr;
>>>>>>>>>> + struct qlist_head tmp_head;
>>>>>>>>>> + unsigned long flags;
>>>>>>>>>>
>>>>>>>>>> if (unlikely(qlist_empty(from)))
>>>>>>>>>> return;
>>>>>>>>>>
>>>>>>>>>> + qlist_init(&tmp_head);
>>>>>>>>>> curr = from->head;
>>>>>>>>>> qlist_init(from);
>>>>>>>>>> while (curr) {
>>>>>>>>>> @@ -278,10 +281,17 @@ static void qlist_move_cache(struct qlist_head *from,
>>>>>>>>>> if (obj_cache == cache)
>>>>>>>>>> qlist_put(to, curr, obj_cache->size);
>>>>>>>>>> else
>>>>>>>>>> - qlist_put(from, curr, obj_cache->size);
>>>>>>>>>> + qlist_put(&tmp_head, curr, obj_cache->size);
>>>>>>>>>>
>>>>>>>>>> curr = next;
>>>>>>>>>> +
>>>>>>>>>> + if (need_resched()) {
>>>>>>>>>> + spin_unlock_irqrestore(&quarantine_lock, flags);
>>>>>>>>>> + cond_resched();
>>>>>>>>>> + spin_lock_irqsave(&quarantine_lock, flags);
>>>>>>>>>> + }
>>>>>>>>>> }
>>>>>>>>>> + qlist_move_all(&tmp_head, from);
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> static void per_cpu_remove_cache(void *arg)
>>>>>>>>>> --
>>>>>>>>>> 2.1.4
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>>>>>>>>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>>>>>>>>> To post to this group, send email to kasan-dev@googlegroups.com.
>>>>>>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CAABZP2zEup53ZcNKOEUEMx_aRMLONZdYCLd7s5J4DLTccPxC-A%40mail.gmail.com.
>>>>>>>>> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 17:56 ` Dmitry Vyukov
@ 2017-11-28 17:57 ` Dmitry Vyukov
2017-11-28 23:41 ` Zhouyi Zhou
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-11-28 17:57 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
On Tue, Nov 28, 2017 at 6:56 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 28, 2017 at 12:30 PM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>> Hi,
>> By using perf top, qlist_move_cache occupies 100% cpu did really
>> happen in my environment yesterday, or I
>> won't notice the kasan code.
>> Currently I have difficulty to let it reappear because the frontend
>> guy modified some user mode code.
>> I can repeat again and again now is
>> kgdb_breakpoint () at kernel/debug/debug_core.c:1073
>> 1073 wmb(); /* Sync point after breakpoint */
>> (gdb) p quarantine_batch_size
>> $1 = 3601946
>> And by instrument code, maximum
>> global_quarantine[quarantine_tail].bytes reached is 6618208.
>
> On second thought, size does not matter too much because there can be
> large objects. Quarantine always quantize by objects, we can't part of
> an object into one batch, and another part of the object into another
> object. But it's not a problem, because overhead per objects is O(1).
> We can push a single 4MB object and overflow target size by 4MB and
> that will be fine.
> Either way, 6MB is not terribly much too. Should take milliseconds to process.
>
>
>
>
>> I do think drain quarantine right in quarantine_put is a better
>> place to drain because cache_free is fine in
>> that context. I am willing do it if you think it is convenient :-)
Andrey, do you know of any problems with draining quarantine in push?
Do you have any objections?
But it's still not completely clear to me what problem we are solving.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 17:57 ` Dmitry Vyukov
@ 2017-11-28 23:41 ` Zhouyi Zhou
2017-11-29 4:54 ` Zhouyi Zhou
0 siblings, 1 reply; 15+ messages in thread
From: Zhouyi Zhou @ 2017-11-28 23:41 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
Hi,
I will try to reestablish the environment, and design proof of
concept of experiment.
Cheers
On Wed, Nov 29, 2017 at 1:57 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 28, 2017 at 6:56 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Nov 28, 2017 at 12:30 PM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>> Hi,
>>> By using perf top, qlist_move_cache occupies 100% cpu did really
>>> happen in my environment yesterday, or I
>>> won't notice the kasan code.
>>> Currently I have difficulty to let it reappear because the frontend
>>> guy modified some user mode code.
>>> I can repeat again and again now is
>>> kgdb_breakpoint () at kernel/debug/debug_core.c:1073
>>> 1073 wmb(); /* Sync point after breakpoint */
>>> (gdb) p quarantine_batch_size
>>> $1 = 3601946
>>> And by instrument code, maximum
>>> global_quarantine[quarantine_tail].bytes reached is 6618208.
>>
>> On second thought, size does not matter too much because there can be
>> large objects. Quarantine always quantize by objects, we can't part of
>> an object into one batch, and another part of the object into another
>> object. But it's not a problem, because overhead per objects is O(1).
>> We can push a single 4MB object and overflow target size by 4MB and
>> that will be fine.
>> Either way, 6MB is not terribly much too. Should take milliseconds to process.
>>
>>
>>
>>
>>> I do think drain quarantine right in quarantine_put is a better
>>> place to drain because cache_free is fine in
>>> that context. I am willing do it if you think it is convenient :-)
>
>
> Andrey, do you know of any problems with draining quarantine in push?
> Do you have any objections?
>
> But it's still not completely clear to me what problem we are solving.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-28 23:41 ` Zhouyi Zhou
@ 2017-11-29 4:54 ` Zhouyi Zhou
2017-11-29 9:03 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Zhouyi Zhou @ 2017-11-29 4:54 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
Hi,
There is new discoveries!
When I find qlist_move_cache reappear in my environment,
I use kgdb to break into function qlist_move_cache. I found
this function is called because of cgroup release.
I also find libvirt allocate a memory croup for each qemu it started,
in my system, it looks like this:
root@ednserver3:/sys/fs/cgroup/memory/machine.slice# ls
cgroup.clone_children machine-qemu\x2d491_25_30.scope
machine-qemu\x2d491_40_30.scope machine-qemu\x2d491_6_30.scope
memory.limit_in_bytes
cgroup.event_control machine-qemu\x2d491_26_30.scope
machine-qemu\x2d491_41_30.scope machine-qemu\x2d491_7_30.scope
memory.max_usage_in_bytes
cgroup.procs machine-qemu\x2d491_27_30.scope
machine-qemu\x2d491_4_30.scope machine-qemu\x2d491_8_30.scope
memory.move_charge_at_immigrate
machine-qemu\x2d491_10_30.scope machine-qemu\x2d491_28_30.scope
machine-qemu\x2d491_47_30.scope machine-qemu\x2d491_9_30.scope
memory.numa_stat
machine-qemu\x2d491_11_30.scope machine-qemu\x2d491_29_30.scope
machine-qemu\x2d491_48_30.scope memory.failcnt
memory.oom_control
machine-qemu\x2d491_12_30.scope machine-qemu\x2d491_30_30.scope
machine-qemu\x2d491_49_30.scope memory.force_empty
memory.pressure_level
machine-qemu\x2d491_13_30.scope machine-qemu\x2d491_31_30.scope
machine-qemu\x2d491_50_30.scope memory.kmem.failcnt
memory.soft_limit_in_bytes
machine-qemu\x2d491_17_30.scope machine-qemu\x2d491_32_30.scope
machine-qemu\x2d491_51_30.scope memory.kmem.limit_in_bytes
memory.stat
machine-qemu\x2d491_18_30.scope machine-qemu\x2d491_33_30.scope
machine-qemu\x2d491_52_30.scope memory.kmem.max_usage_in_bytes
memory.swappiness
machine-qemu\x2d491_19_30.scope machine-qemu\x2d491_34_30.scope
machine-qemu\x2d491_5_30.scope memory.kmem.slabinfo
memory.usage_in_bytes
machine-qemu\x2d491_20_30.scope machine-qemu\x2d491_35_30.scope
machine-qemu\x2d491_53_30.scope memory.kmem.tcp.failcnt
memory.use_hierarchy
machine-qemu\x2d491_21_30.scope machine-qemu\x2d491_36_30.scope
machine-qemu\x2d491_54_30.scope memory.kmem.tcp.limit_in_bytes
notify_on_release
machine-qemu\x2d491_22_30.scope machine-qemu\x2d491_37_30.scope
machine-qemu\x2d491_55_30.scope memory.kmem.tcp.max_usage_in_bytes
tasks
machine-qemu\x2d491_23_30.scope machine-qemu\x2d491_38_30.scope
machine-qemu\x2d491_56_30.scope memory.kmem.tcp.usage_in_bytes
machine-qemu\x2d491_24_30.scope machine-qemu\x2d491_39_30.scope
machine-qemu\x2d491_57_30.scope memory.kmem.usage_in_bytes
and in each memory cgroup there are many slabs:
root@ednserver3:/sys/fs/cgroup/memory/machine.slice/machine-qemu\x2d491_10_30.scope#
cat memory.kmem.slabinfo
slabinfo - version: 2.1
# name <active_objs> <num_objs> <objsize> <objperslab>
<pagesperslab> : tunables <limit> <batchcount> <sharedfactor> :
slabdata <active_slabs> <num_slabs> <sharedavail>
kmalloc-2048 0 0 2240 3 2 : tunables 24 12
8 : slabdata 0 0 0
kmalloc-512 0 0 704 11 2 : tunables 54 27
8 : slabdata 0 0 0
skbuff_head_cache 0 0 384 10 1 : tunables 54 27
8 : slabdata 0 0 0
kmalloc-1024 0 0 1216 3 1 : tunables 24 12
8 : slabdata 0 0 0
kmalloc-192 0 0 320 12 1 : tunables 120 60
8 : slabdata 0 0 0
pid 3 21 192 21 1 : tunables 120 60
8 : slabdata 1 1 0
signal_cache 0 0 1216 3 1 : tunables 24 12
8 : slabdata 0 0 0
sighand_cache 0 0 2304 3 2 : tunables 24 12
8 : slabdata 0 0 0
fs_cache 0 0 192 21 1 : tunables 120 60
8 : slabdata 0 0 0
files_cache 0 0 896 4 1 : tunables 54 27
8 : slabdata 0 0 0
task_delay_info 3 72 112 36 1 : tunables 120 60
8 : slabdata 2 2 0
task_struct 3 3 3840 1 1 : tunables 24 12
8 : slabdata 3 3 0
radix_tree_node 0 0 728 5 1 : tunables 54 27
8 : slabdata 0 0 0
shmem_inode_cache 2 9 848 9 2 : tunables 54 27
8 : slabdata 1 1 0
inode_cache 39 45 744 5 1 : tunables 54 27
8 : slabdata 9 9 0
ext4_inode_cache 0 0 1224 3 1 : tunables 24 12
8 : slabdata 0 0 0
sock_inode_cache 3 8 832 4 1 : tunables 54 27
8 : slabdata 2 2 0
proc_inode_cache 0 0 816 5 1 : tunables 54 27
8 : slabdata 0 0 0
dentry 52 90 272 15 1 : tunables 120 60
8 : slabdata 6 6 0
anon_vma 140 348 136 29 1 : tunables 120 60
8 : slabdata 12 12 0
anon_vma_chain 257 468 112 36 1 : tunables 120 60
8 : slabdata 13 13 0
vm_area_struct 510 780 272 15 1 : tunables 120 60
8 : slabdata 52 52 0
mm_struct 1 3 1280 3 1 : tunables 24 12
8 : slabdata 1 1 0
cred_jar 12 24 320 12 1 : tunables 120 60
8 : slabdata 2 2 0
So, when I end the libvirt scenery, those slabs belong to those qemus
has to invoke quarantine_remove_cache,
I guess that's why qlist_move_cache occupies so much CPU cycles. I
also guess this make libvirt complain
(wait for too long?)
Sorry not to research deeply into system in the first place and submit
a patch in a hurry.
And I propose a little sugguestion to improve qlist_move_cache if you
like. Won't we design some kind of hash mechanism,
then we group the qlist_node according to their cache, so as not to
compare one by one to every qlist_node in the system.
Sorry for your time
Best Wishes
Zhouyi
On Wed, Nov 29, 2017 at 7:41 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Hi,
> I will try to reestablish the environment, and design proof of
> concept of experiment.
> Cheers
>
> On Wed, Nov 29, 2017 at 1:57 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Nov 28, 2017 at 6:56 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> On Tue, Nov 28, 2017 at 12:30 PM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>>>> Hi,
>>>> By using perf top, qlist_move_cache occupies 100% cpu did really
>>>> happen in my environment yesterday, or I
>>>> won't notice the kasan code.
>>>> Currently I have difficulty to let it reappear because the frontend
>>>> guy modified some user mode code.
>>>> I can repeat again and again now is
>>>> kgdb_breakpoint () at kernel/debug/debug_core.c:1073
>>>> 1073 wmb(); /* Sync point after breakpoint */
>>>> (gdb) p quarantine_batch_size
>>>> $1 = 3601946
>>>> And by instrument code, maximum
>>>> global_quarantine[quarantine_tail].bytes reached is 6618208.
>>>
>>> On second thought, size does not matter too much because there can be
>>> large objects. Quarantine always quantize by objects, we can't part of
>>> an object into one batch, and another part of the object into another
>>> object. But it's not a problem, because overhead per objects is O(1).
>>> We can push a single 4MB object and overflow target size by 4MB and
>>> that will be fine.
>>> Either way, 6MB is not terribly much too. Should take milliseconds to process.
>>>
>>>
>>>
>>>
>>>> I do think drain quarantine right in quarantine_put is a better
>>>> place to drain because cache_free is fine in
>>>> that context. I am willing do it if you think it is convenient :-)
>>
>>
>> Andrey, do you know of any problems with draining quarantine in push?
>> Do you have any objections?
>>
>> But it's still not completely clear to me what problem we are solving.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] kasan: fix livelock in qlist_move_cache
2017-11-29 4:54 ` Zhouyi Zhou
@ 2017-11-29 9:03 ` Dmitry Vyukov
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2017-11-29 9:03 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: Andrey Ryabinin, Alexander Potapenko, kasan-dev, Linux-MM, linux-kernel
On Wed, Nov 29, 2017 at 5:54 AM, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Hi,
> There is new discoveries!
>
> When I find qlist_move_cache reappear in my environment,
> I use kgdb to break into function qlist_move_cache. I found
> this function is called because of cgroup release.
>
> I also find libvirt allocate a memory croup for each qemu it started,
> in my system, it looks like this:
>
> root@ednserver3:/sys/fs/cgroup/memory/machine.slice# ls
> cgroup.clone_children machine-qemu\x2d491_25_30.scope
> machine-qemu\x2d491_40_30.scope machine-qemu\x2d491_6_30.scope
> memory.limit_in_bytes
> cgroup.event_control machine-qemu\x2d491_26_30.scope
> machine-qemu\x2d491_41_30.scope machine-qemu\x2d491_7_30.scope
> memory.max_usage_in_bytes
> cgroup.procs machine-qemu\x2d491_27_30.scope
> machine-qemu\x2d491_4_30.scope machine-qemu\x2d491_8_30.scope
> memory.move_charge_at_immigrate
> machine-qemu\x2d491_10_30.scope machine-qemu\x2d491_28_30.scope
> machine-qemu\x2d491_47_30.scope machine-qemu\x2d491_9_30.scope
> memory.numa_stat
> machine-qemu\x2d491_11_30.scope machine-qemu\x2d491_29_30.scope
> machine-qemu\x2d491_48_30.scope memory.failcnt
> memory.oom_control
> machine-qemu\x2d491_12_30.scope machine-qemu\x2d491_30_30.scope
> machine-qemu\x2d491_49_30.scope memory.force_empty
> memory.pressure_level
> machine-qemu\x2d491_13_30.scope machine-qemu\x2d491_31_30.scope
> machine-qemu\x2d491_50_30.scope memory.kmem.failcnt
> memory.soft_limit_in_bytes
> machine-qemu\x2d491_17_30.scope machine-qemu\x2d491_32_30.scope
> machine-qemu\x2d491_51_30.scope memory.kmem.limit_in_bytes
> memory.stat
> machine-qemu\x2d491_18_30.scope machine-qemu\x2d491_33_30.scope
> machine-qemu\x2d491_52_30.scope memory.kmem.max_usage_in_bytes
> memory.swappiness
> machine-qemu\x2d491_19_30.scope machine-qemu\x2d491_34_30.scope
> machine-qemu\x2d491_5_30.scope memory.kmem.slabinfo
> memory.usage_in_bytes
> machine-qemu\x2d491_20_30.scope machine-qemu\x2d491_35_30.scope
> machine-qemu\x2d491_53_30.scope memory.kmem.tcp.failcnt
> memory.use_hierarchy
> machine-qemu\x2d491_21_30.scope machine-qemu\x2d491_36_30.scope
> machine-qemu\x2d491_54_30.scope memory.kmem.tcp.limit_in_bytes
> notify_on_release
> machine-qemu\x2d491_22_30.scope machine-qemu\x2d491_37_30.scope
> machine-qemu\x2d491_55_30.scope memory.kmem.tcp.max_usage_in_bytes
> tasks
> machine-qemu\x2d491_23_30.scope machine-qemu\x2d491_38_30.scope
> machine-qemu\x2d491_56_30.scope memory.kmem.tcp.usage_in_bytes
> machine-qemu\x2d491_24_30.scope machine-qemu\x2d491_39_30.scope
> machine-qemu\x2d491_57_30.scope memory.kmem.usage_in_bytes
>
> and in each memory cgroup there are many slabs:
> root@ednserver3:/sys/fs/cgroup/memory/machine.slice/machine-qemu\x2d491_10_30.scope#
> cat memory.kmem.slabinfo
> slabinfo - version: 2.1
> # name <active_objs> <num_objs> <objsize> <objperslab>
> <pagesperslab> : tunables <limit> <batchcount> <sharedfactor> :
> slabdata <active_slabs> <num_slabs> <sharedavail>
> kmalloc-2048 0 0 2240 3 2 : tunables 24 12
> 8 : slabdata 0 0 0
> kmalloc-512 0 0 704 11 2 : tunables 54 27
> 8 : slabdata 0 0 0
> skbuff_head_cache 0 0 384 10 1 : tunables 54 27
> 8 : slabdata 0 0 0
> kmalloc-1024 0 0 1216 3 1 : tunables 24 12
> 8 : slabdata 0 0 0
> kmalloc-192 0 0 320 12 1 : tunables 120 60
> 8 : slabdata 0 0 0
> pid 3 21 192 21 1 : tunables 120 60
> 8 : slabdata 1 1 0
> signal_cache 0 0 1216 3 1 : tunables 24 12
> 8 : slabdata 0 0 0
> sighand_cache 0 0 2304 3 2 : tunables 24 12
> 8 : slabdata 0 0 0
> fs_cache 0 0 192 21 1 : tunables 120 60
> 8 : slabdata 0 0 0
> files_cache 0 0 896 4 1 : tunables 54 27
> 8 : slabdata 0 0 0
> task_delay_info 3 72 112 36 1 : tunables 120 60
> 8 : slabdata 2 2 0
> task_struct 3 3 3840 1 1 : tunables 24 12
> 8 : slabdata 3 3 0
> radix_tree_node 0 0 728 5 1 : tunables 54 27
> 8 : slabdata 0 0 0
> shmem_inode_cache 2 9 848 9 2 : tunables 54 27
> 8 : slabdata 1 1 0
> inode_cache 39 45 744 5 1 : tunables 54 27
> 8 : slabdata 9 9 0
> ext4_inode_cache 0 0 1224 3 1 : tunables 24 12
> 8 : slabdata 0 0 0
> sock_inode_cache 3 8 832 4 1 : tunables 54 27
> 8 : slabdata 2 2 0
> proc_inode_cache 0 0 816 5 1 : tunables 54 27
> 8 : slabdata 0 0 0
> dentry 52 90 272 15 1 : tunables 120 60
> 8 : slabdata 6 6 0
> anon_vma 140 348 136 29 1 : tunables 120 60
> 8 : slabdata 12 12 0
> anon_vma_chain 257 468 112 36 1 : tunables 120 60
> 8 : slabdata 13 13 0
> vm_area_struct 510 780 272 15 1 : tunables 120 60
> 8 : slabdata 52 52 0
> mm_struct 1 3 1280 3 1 : tunables 24 12
> 8 : slabdata 1 1 0
> cred_jar 12 24 320 12 1 : tunables 120 60
> 8 : slabdata 2 2 0
>
> So, when I end the libvirt scenery, those slabs belong to those qemus
> has to invoke quarantine_remove_cache,
> I guess that's why qlist_move_cache occupies so much CPU cycles. I
> also guess this make libvirt complain
> (wait for too long?)
>
> Sorry not to research deeply into system in the first place and submit
> a patch in a hurry.
>
> And I propose a little sugguestion to improve qlist_move_cache if you
> like. Won't we design some kind of hash mechanism,
> then we group the qlist_node according to their cache, so as not to
> compare one by one to every qlist_node in the system.
Yes, quarantine_remove_cache() is very slow because it walk a huge
linked list and synchronize_srcu() does not help either. It would be
great to make it faster rather than peppering over the problem with
rescheds.
Please detail your scheme.
Note that quarantine needs to be [best-effort] global FIFO and that
the main operations are actually kmalloc/kfree, so we should not
penalize them either. We also have limited memory in memory blocks.
I had some ideas but I couldn't come up with a complete solution that
I would like.
One thing is that we could first check if the cache actually has _any_
outstanding objects. Looking at your slabinfo dump, it seems that lots
of them don't have active objects. In that case we can skip all of
quarantine_remove_cache entirely. I see there is already a function
for this:
static int shutdown_cache(struct kmem_cache *s)
{
/* free asan quarantined objects */
kasan_cache_shutdown(s);
if (__kmem_cache_shutdown(s) != 0)
return -EBUSY;
So maybe we could do just:
static int shutdown_cache(struct kmem_cache *s)
{
if (__kmem_cache_shutdown(s) != 0) {
/* free asan quarantined objects */
kasan_cache_shutdown(s);
if (__kmem_cache_shutdown(s) != 0)
return -EBUSY;
}
We could also make cache freeing asynchronous. Then we could either
just wait when the cache doesn't have any active objects (walk and
check all deferred caches after each quarantine_reduce()), or
accumulate a batch of them and then walk quarantine once and remove
objects for the batch of caches (this would amortize overhead by batch
size). As far as I understand in lots of cases caches are freed in
large batches (cgroups, namespaces), and that's exactly when
quarantine_remove_cache() performance is a problem.
Or we could make quarantine a doubly-linked list and then walk all
active objects in the cache (is it possible?) and remove them from
quarantine by shuffling next/prev pointers. However, this can increase
memory consumption and penalize performance of other operations.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-11-29 9:03 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-28 4:04 [PATCH 1/1] kasan: fix livelock in qlist_move_cache zhouzhouyi
2017-11-28 4:05 ` Zhouyi Zhou
2017-11-28 7:45 ` Dmitry Vyukov
2017-11-28 8:00 ` Zhouyi Zhou
2017-11-28 8:12 ` Dmitry Vyukov
2017-11-28 8:33 ` Zhouyi Zhou
2017-11-28 8:58 ` Dmitry Vyukov
2017-11-28 9:17 ` Zhouyi Zhou
2017-11-28 9:27 ` Dmitry Vyukov
2017-11-28 11:30 ` Zhouyi Zhou
2017-11-28 17:56 ` Dmitry Vyukov
2017-11-28 17:57 ` Dmitry Vyukov
2017-11-28 23:41 ` Zhouyi Zhou
2017-11-29 4:54 ` Zhouyi Zhou
2017-11-29 9:03 ` Dmitry Vyukov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).