Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1335713 > unrolled thread

Re: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable stackdepot for SLAB

Started byAlexander Potapenko <glider@google.com>
First post2016-02-16 19:40 +0100
Last post2016-02-18 09:00 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable  stackdepot for SLAB Alexander Potapenko <glider@google.com> - 2016-02-16 19:40 +0100
    Re: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable  stackdepot for SLAB Alexander Potapenko <glider@google.com> - 2016-02-17 19:30 +0100
      Re: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable  stackdepot for SLAB Joonsoo Kim <js1304@gmail.com> - 2016-02-18 09:20 +0100
        Re: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable  stackdepot for SLAB Alexander Potapenko <glider@google.com> - 2016-02-18 16:10 +0100
    Re: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable  stackdepot for SLAB Joonsoo Kim <js1304@gmail.com> - 2016-02-18 09:00 +0100

#1335713 — Re: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable stackdepot for SLAB

FromAlexander Potapenko <glider@google.com>
Date2016-02-16 19:40 +0100
SubjectRe: [PATCH v1 5/8] mm, kasan: Stackdepot implementation. Enable stackdepot for SLAB
Message-ID<r2SHv-3a7-9@gated-at.bofh.it>
On Mon, Feb 1, 2016 at 3:55 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
> On Thu, Jan 28, 2016 at 02:27:44PM +0100, Alexander Potapenko wrote:
>> On Thu, Jan 28, 2016 at 1:51 PM, Alexander Potapenko <glider@google.com> wrote:
>> >
>> > On Jan 28, 2016 8:40 AM, "Joonsoo Kim" <iamjoonsoo.kim@lge.com> wrote:
>> >>
>> >> Hello,
>> >>
>> >> On Wed, Jan 27, 2016 at 07:25:10PM +0100, Alexander Potapenko wrote:
>> >> > Stack depot will allow KASAN store allocation/deallocation stack traces
>> >> > for memory chunks. The stack traces are stored in a hash table and
>> >> > referenced by handles which reside in the kasan_alloc_meta and
>> >> > kasan_free_meta structures in the allocated memory chunks.
>> >>
>> >> Looks really nice!
>> >>
>> >> Could it be more generalized to be used by other feature that need to
>> >> store stack trace such as tracepoint or page owner?
>> > Certainly yes, but see below.
>> >
>> >> If it could be, there is one more requirement.
>> >> I understand the fact that entry is never removed from depot makes things
>> >> very simpler, but, for general usecases, it's better to use reference
>> >> count
>> >> and allow to remove. Is it possible?
>> > For our use case reference counting is not really necessary, and it would
>> > introduce unwanted contention.
>
> Okay.
>
>> > There are two possible options, each having its advantages and drawbacks: we
>> > can let the clients store the refcounters directly in their stacks (more
>> > universal, but harder to use for the clients), or keep the counters in the
>> > depot but add an API that does not change them (easier for the clients, but
>> > potentially error-prone).
>> > I'd say it's better to actually find at least one more user for the stack
>> > depot in order to understand the requirements, and refactor the code after
>> > that.
>
> I re-think the page owner case and it also may not need refcount.
> For now, just moving this stuff to /lib would be helpful for other future user.
I agree this code may need to be moved to /lib someday, but I wouldn't
hurry with that.
Right now it is quite KASAN-specific, and it's unclear yet whether
anyone else is going to use it.
I suggest we keep it in mm/kasan for now, and factor the common parts
into /lib when the need arises.

> BTW, is there any performance number? I guess that it could affect
> the performance.
I've compared the performance of KASAN with SLAB allocator on a small
synthetic benchmark in two modes: with stack depot enabled and with
kasan_save_stack() unconditionally returning 0.
In the former case 8% more time was spent in the kernel than in the latter case.

If I am not mistaking, for SLUB allocator the bookkeeping (enabled
with the slub_debug=UZ boot options) take only 1.5 time, so the
difference is worth looking into (at least before we switch SLUB to
stack depot).


> Thanks.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

[toc] | [next] | [standalone]


#1336638

FromAlexander Potapenko <glider@google.com>
Date2016-02-17 19:30 +0100
Message-ID<r3f1p-1O1-49@gated-at.bofh.it>
In reply to#1335713
On Tue, Feb 16, 2016 at 7:37 PM, Alexander Potapenko <glider@google.com> wrote:
> On Mon, Feb 1, 2016 at 3:55 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
>> On Thu, Jan 28, 2016 at 02:27:44PM +0100, Alexander Potapenko wrote:
>>> On Thu, Jan 28, 2016 at 1:51 PM, Alexander Potapenko <glider@google.com> wrote:
>>> >
>>> > On Jan 28, 2016 8:40 AM, "Joonsoo Kim" <iamjoonsoo.kim@lge.com> wrote:
>>> >>
>>> >> Hello,
>>> >>
>>> >> On Wed, Jan 27, 2016 at 07:25:10PM +0100, Alexander Potapenko wrote:
>>> >> > Stack depot will allow KASAN store allocation/deallocation stack traces
>>> >> > for memory chunks. The stack traces are stored in a hash table and
>>> >> > referenced by handles which reside in the kasan_alloc_meta and
>>> >> > kasan_free_meta structures in the allocated memory chunks.
>>> >>
>>> >> Looks really nice!
>>> >>
>>> >> Could it be more generalized to be used by other feature that need to
>>> >> store stack trace such as tracepoint or page owner?
>>> > Certainly yes, but see below.
>>> >
>>> >> If it could be, there is one more requirement.
>>> >> I understand the fact that entry is never removed from depot makes things
>>> >> very simpler, but, for general usecases, it's better to use reference
>>> >> count
>>> >> and allow to remove. Is it possible?
>>> > For our use case reference counting is not really necessary, and it would
>>> > introduce unwanted contention.
>>
>> Okay.
>>
>>> > There are two possible options, each having its advantages and drawbacks: we
>>> > can let the clients store the refcounters directly in their stacks (more
>>> > universal, but harder to use for the clients), or keep the counters in the
>>> > depot but add an API that does not change them (easier for the clients, but
>>> > potentially error-prone).
>>> > I'd say it's better to actually find at least one more user for the stack
>>> > depot in order to understand the requirements, and refactor the code after
>>> > that.
>>
>> I re-think the page owner case and it also may not need refcount.
>> For now, just moving this stuff to /lib would be helpful for other future user.
> I agree this code may need to be moved to /lib someday, but I wouldn't
> hurry with that.
> Right now it is quite KASAN-specific, and it's unclear yet whether
> anyone else is going to use it.
> I suggest we keep it in mm/kasan for now, and factor the common parts
> into /lib when the need arises.
>
>> BTW, is there any performance number? I guess that it could affect
>> the performance.
> I've compared the performance of KASAN with SLAB allocator on a small
> synthetic benchmark in two modes: with stack depot enabled and with
> kasan_save_stack() unconditionally returning 0.
> In the former case 8% more time was spent in the kernel than in the latter case.
>
> If I am not mistaking, for SLUB allocator the bookkeeping (enabled
> with the slub_debug=UZ boot options) take only 1.5 time, so the
> difference is worth looking into (at least before we switch SLUB to
> stack depot).

I've made additional measurements.
Previously I had been using a userspace benchmark that created and
destroyed pipes in a loop
(https://github.com/google/sanitizers/blob/master/address-sanitizer/kernel_buildbot/slave/bench_pipes.c).

Now I've made a kernel module that allocated and deallocated memory
chunks of different sizes in a loop.
There were two modes of operation:
1) all the allocations were made from the same function, therefore all
allocation/deallocation stacks were similar and there always was a hit
in the stackdepot hashtable
2) The allocations were made from 2^16 different stacks.

In the first case SLAB+stackdepot turned out to be 13% faster than
SLUB+slub_debug, in the second SLAB was 11% faster.
Note that in both cases and for both allocators most of the time (more
than 90%) was spent in the x86 stack unwinder, which is common for
both approaches.

Yet another observation regarding stackdepot: under a heavy load
(running Trinity for a hour, 101M allocations) the depot saturates at
around 20K records with the hashtable miss rate of 0.02%.
That said, I still cannot justify the results of the userspace
benchmark, but the slowdown of the stackdepot approach for SLAB sounds
acceptable, especially given the memory gain compared to SLUB
bookkeeping (which requires 128 bytes per memory allocation) and the
fact we'll be dealing with the fast path most of the time.

It will certainly be nice to compare SLUB+slub_debug to
SLUB+stackdepot once we start switching SLUB to stackdepot.


>
>> Thanks.
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
> leiten Sie diese bitte nicht weiter, informieren Sie den
> Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
> This e-mail is confidential. If you are not the right addressee please
> do not forward it, please inform the sender, and please erase this
> e-mail including any attachments. Thanks.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

[toc] | [prev] | [next] | [standalone]


#1337123

FromJoonsoo Kim <js1304@gmail.com>
Date2016-02-18 09:20 +0100
Message-ID<r3rYB-2K0-5@gated-at.bofh.it>
In reply to#1336638
2016-02-18 3:29 GMT+09:00 Alexander Potapenko <glider@google.com>:
> On Tue, Feb 16, 2016 at 7:37 PM, Alexander Potapenko <glider@google.com> wrote:
>> On Mon, Feb 1, 2016 at 3:55 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
>>> On Thu, Jan 28, 2016 at 02:27:44PM +0100, Alexander Potapenko wrote:
>>>> On Thu, Jan 28, 2016 at 1:51 PM, Alexander Potapenko <glider@google.com> wrote:
>>>> >
>>>> > On Jan 28, 2016 8:40 AM, "Joonsoo Kim" <iamjoonsoo.kim@lge.com> wrote:
>>>> >>
>>>> >> Hello,
>>>> >>
>>>> >> On Wed, Jan 27, 2016 at 07:25:10PM +0100, Alexander Potapenko wrote:
>>>> >> > Stack depot will allow KASAN store allocation/deallocation stack traces
>>>> >> > for memory chunks. The stack traces are stored in a hash table and
>>>> >> > referenced by handles which reside in the kasan_alloc_meta and
>>>> >> > kasan_free_meta structures in the allocated memory chunks.
>>>> >>
>>>> >> Looks really nice!
>>>> >>
>>>> >> Could it be more generalized to be used by other feature that need to
>>>> >> store stack trace such as tracepoint or page owner?
>>>> > Certainly yes, but see below.
>>>> >
>>>> >> If it could be, there is one more requirement.
>>>> >> I understand the fact that entry is never removed from depot makes things
>>>> >> very simpler, but, for general usecases, it's better to use reference
>>>> >> count
>>>> >> and allow to remove. Is it possible?
>>>> > For our use case reference counting is not really necessary, and it would
>>>> > introduce unwanted contention.
>>>
>>> Okay.
>>>
>>>> > There are two possible options, each having its advantages and drawbacks: we
>>>> > can let the clients store the refcounters directly in their stacks (more
>>>> > universal, but harder to use for the clients), or keep the counters in the
>>>> > depot but add an API that does not change them (easier for the clients, but
>>>> > potentially error-prone).
>>>> > I'd say it's better to actually find at least one more user for the stack
>>>> > depot in order to understand the requirements, and refactor the code after
>>>> > that.
>>>
>>> I re-think the page owner case and it also may not need refcount.
>>> For now, just moving this stuff to /lib would be helpful for other future user.
>> I agree this code may need to be moved to /lib someday, but I wouldn't
>> hurry with that.
>> Right now it is quite KASAN-specific, and it's unclear yet whether
>> anyone else is going to use it.
>> I suggest we keep it in mm/kasan for now, and factor the common parts
>> into /lib when the need arises.
>>
>>> BTW, is there any performance number? I guess that it could affect
>>> the performance.
>> I've compared the performance of KASAN with SLAB allocator on a small
>> synthetic benchmark in two modes: with stack depot enabled and with
>> kasan_save_stack() unconditionally returning 0.
>> In the former case 8% more time was spent in the kernel than in the latter case.
>>
>> If I am not mistaking, for SLUB allocator the bookkeeping (enabled
>> with the slub_debug=UZ boot options) take only 1.5 time, so the
>> difference is worth looking into (at least before we switch SLUB to
>> stack depot).
>
> I've made additional measurements.
> Previously I had been using a userspace benchmark that created and
> destroyed pipes in a loop
> (https://github.com/google/sanitizers/blob/master/address-sanitizer/kernel_buildbot/slave/bench_pipes.c).
>
> Now I've made a kernel module that allocated and deallocated memory
> chunks of different sizes in a loop.
> There were two modes of operation:
> 1) all the allocations were made from the same function, therefore all
> allocation/deallocation stacks were similar and there always was a hit
> in the stackdepot hashtable
> 2) The allocations were made from 2^16 different stacks.
>
> In the first case SLAB+stackdepot turned out to be 13% faster than
> SLUB+slub_debug, in the second SLAB was 11% faster.

I don't know what version of kernel you tested but, until recently,
slub_debug=UZ has a side effect not to using fastpath of SLUB. So,
comparison between them isn't appropriate. Today's linux-next branch
would have some improvements on this area so use it to compare them.

> Note that in both cases and for both allocators most of the time (more
> than 90%) was spent in the x86 stack unwinder, which is common for
> both approaches.

If more than 90% time is spent in stack unwinder which is common for
both cases, how something is better than the other by 13%?

> Yet another observation regarding stackdepot: under a heavy load
> (running Trinity for a hour, 101M allocations) the depot saturates at
> around 20K records with the hashtable miss rate of 0.02%.
> That said, I still cannot justify the results of the userspace
> benchmark, but the slowdown of the stackdepot approach for SLAB sounds
> acceptable, especially given the memory gain compared to SLUB
> bookkeeping (which requires 128 bytes per memory allocation) and the
> fact we'll be dealing with the fast path most of the time.

In fact, I don't have much concern about performance because saving
memory has enough merit to be merged. Anyway, it looks acceptable
even for performance.

> It will certainly be nice to compare SLUB+slub_debug to
> SLUB+stackdepot once we start switching SLUB to stackdepot.

Okay.

Thanks.

[toc] | [prev] | [next] | [standalone]


#1337442

FromAlexander Potapenko <glider@google.com>
Date2016-02-18 16:10 +0100
Message-ID<r3yno-7re-13@gated-at.bofh.it>
In reply to#1337123
On Thu, Feb 18, 2016 at 9:13 AM, Joonsoo Kim <js1304@gmail.com> wrote:
> 2016-02-18 3:29 GMT+09:00 Alexander Potapenko <glider@google.com>:
>> On Tue, Feb 16, 2016 at 7:37 PM, Alexander Potapenko <glider@google.com> wrote:
>>> On Mon, Feb 1, 2016 at 3:55 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
>>>> On Thu, Jan 28, 2016 at 02:27:44PM +0100, Alexander Potapenko wrote:
>>>>> On Thu, Jan 28, 2016 at 1:51 PM, Alexander Potapenko <glider@google.com> wrote:
>>>>> >
>>>>> > On Jan 28, 2016 8:40 AM, "Joonsoo Kim" <iamjoonsoo.kim@lge.com> wrote:
>>>>> >>
>>>>> >> Hello,
>>>>> >>
>>>>> >> On Wed, Jan 27, 2016 at 07:25:10PM +0100, Alexander Potapenko wrote:
>>>>> >> > Stack depot will allow KASAN store allocation/deallocation stack traces
>>>>> >> > for memory chunks. The stack traces are stored in a hash table and
>>>>> >> > referenced by handles which reside in the kasan_alloc_meta and
>>>>> >> > kasan_free_meta structures in the allocated memory chunks.
>>>>> >>
>>>>> >> Looks really nice!
>>>>> >>
>>>>> >> Could it be more generalized to be used by other feature that need to
>>>>> >> store stack trace such as tracepoint or page owner?
>>>>> > Certainly yes, but see below.
>>>>> >
>>>>> >> If it could be, there is one more requirement.
>>>>> >> I understand the fact that entry is never removed from depot makes things
>>>>> >> very simpler, but, for general usecases, it's better to use reference
>>>>> >> count
>>>>> >> and allow to remove. Is it possible?
>>>>> > For our use case reference counting is not really necessary, and it would
>>>>> > introduce unwanted contention.
>>>>
>>>> Okay.
>>>>
>>>>> > There are two possible options, each having its advantages and drawbacks: we
>>>>> > can let the clients store the refcounters directly in their stacks (more
>>>>> > universal, but harder to use for the clients), or keep the counters in the
>>>>> > depot but add an API that does not change them (easier for the clients, but
>>>>> > potentially error-prone).
>>>>> > I'd say it's better to actually find at least one more user for the stack
>>>>> > depot in order to understand the requirements, and refactor the code after
>>>>> > that.
>>>>
>>>> I re-think the page owner case and it also may not need refcount.
>>>> For now, just moving this stuff to /lib would be helpful for other future user.
>>> I agree this code may need to be moved to /lib someday, but I wouldn't
>>> hurry with that.
>>> Right now it is quite KASAN-specific, and it's unclear yet whether
>>> anyone else is going to use it.
>>> I suggest we keep it in mm/kasan for now, and factor the common parts
>>> into /lib when the need arises.
>>>
>>>> BTW, is there any performance number? I guess that it could affect
>>>> the performance.
>>> I've compared the performance of KASAN with SLAB allocator on a small
>>> synthetic benchmark in two modes: with stack depot enabled and with
>>> kasan_save_stack() unconditionally returning 0.
>>> In the former case 8% more time was spent in the kernel than in the latter case.
>>>
>>> If I am not mistaking, for SLUB allocator the bookkeeping (enabled
>>> with the slub_debug=UZ boot options) take only 1.5 time, so the
>>> difference is worth looking into (at least before we switch SLUB to
>>> stack depot).
>>
>> I've made additional measurements.
>> Previously I had been using a userspace benchmark that created and
>> destroyed pipes in a loop
>> (https://github.com/google/sanitizers/blob/master/address-sanitizer/kernel_buildbot/slave/bench_pipes.c).
>>
>> Now I've made a kernel module that allocated and deallocated memory
>> chunks of different sizes in a loop.
>> There were two modes of operation:
>> 1) all the allocations were made from the same function, therefore all
>> allocation/deallocation stacks were similar and there always was a hit
>> in the stackdepot hashtable
>> 2) The allocations were made from 2^16 different stacks.
>>
>> In the first case SLAB+stackdepot turned out to be 13% faster than
>> SLUB+slub_debug, in the second SLAB was 11% faster.
>
> I don't know what version of kernel you tested but, until recently,
> slub_debug=UZ has a side effect not to using fastpath of SLUB. So,
> comparison between them isn't appropriate. Today's linux-next branch
> would have some improvements on this area so use it to compare them.
>
That's good to know.
I've been using https://github.com/torvalds/linux.git, which probably
didn't have those improvements.

>> Note that in both cases and for both allocators most of the time (more
>> than 90%) was spent in the x86 stack unwinder, which is common for
>> both approaches.
>
> If more than 90% time is spent in stack unwinder which is common for
> both cases, how something is better than the other by 13%?
On the second glance, this number (90%) may be inaccurate, because I
measured the stack unwinding times separately, which could have
introduced deviation (not to mention it was incorrect for SLUB).
Yet we're talking about a significant amount of time spent in the unwinder.
My numbers were 26.111 seconds for 1024K SLAB allocation/deallocation
pairs and 30.278 seconds for 1024K alloc/dealloc pairs with SLUB.
When measured separately in the same routine that did the allocations,
2048K calls to save_stack_trace() took 25.487 seconds.

>> Yet another observation regarding stackdepot: under a heavy load
>> (running Trinity for a hour, 101M allocations) the depot saturates at
>> around 20K records with the hashtable miss rate of 0.02%.
>> That said, I still cannot justify the results of the userspace
>> benchmark, but the slowdown of the stackdepot approach for SLAB sounds
>> acceptable, especially given the memory gain compared to SLUB
>> bookkeeping (which requires 128 bytes per memory allocation) and the
>> fact we'll be dealing with the fast path most of the time.
>
> In fact, I don't have much concern about performance because saving
> memory has enough merit to be merged. Anyway, it looks acceptable
> even for performance.
>
>> It will certainly be nice to compare SLUB+slub_debug to
>> SLUB+stackdepot once we start switching SLUB to stackdepot.
>
> Okay.
>
> Thanks.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

[toc] | [prev] | [next] | [standalone]


#1337106

FromJoonsoo Kim <js1304@gmail.com>
Date2016-02-18 09:00 +0100
Message-ID<r3rFg-2m5-1@gated-at.bofh.it>
In reply to#1335713
2016-02-17 3:37 GMT+09:00 Alexander Potapenko <glider@google.com>:
> On Mon, Feb 1, 2016 at 3:55 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
>> On Thu, Jan 28, 2016 at 02:27:44PM +0100, Alexander Potapenko wrote:
>>> On Thu, Jan 28, 2016 at 1:51 PM, Alexander Potapenko <glider@google.com> wrote:
>>> >
>>> > On Jan 28, 2016 8:40 AM, "Joonsoo Kim" <iamjoonsoo.kim@lge.com> wrote:
>>> >>
>>> >> Hello,
>>> >>
>>> >> On Wed, Jan 27, 2016 at 07:25:10PM +0100, Alexander Potapenko wrote:
>>> >> > Stack depot will allow KASAN store allocation/deallocation stack traces
>>> >> > for memory chunks. The stack traces are stored in a hash table and
>>> >> > referenced by handles which reside in the kasan_alloc_meta and
>>> >> > kasan_free_meta structures in the allocated memory chunks.
>>> >>
>>> >> Looks really nice!
>>> >>
>>> >> Could it be more generalized to be used by other feature that need to
>>> >> store stack trace such as tracepoint or page owner?
>>> > Certainly yes, but see below.
>>> >
>>> >> If it could be, there is one more requirement.
>>> >> I understand the fact that entry is never removed from depot makes things
>>> >> very simpler, but, for general usecases, it's better to use reference
>>> >> count
>>> >> and allow to remove. Is it possible?
>>> > For our use case reference counting is not really necessary, and it would
>>> > introduce unwanted contention.
>>
>> Okay.
>>
>>> > There are two possible options, each having its advantages and drawbacks: we
>>> > can let the clients store the refcounters directly in their stacks (more
>>> > universal, but harder to use for the clients), or keep the counters in the
>>> > depot but add an API that does not change them (easier for the clients, but
>>> > potentially error-prone).
>>> > I'd say it's better to actually find at least one more user for the stack
>>> > depot in order to understand the requirements, and refactor the code after
>>> > that.
>>
>> I re-think the page owner case and it also may not need refcount.
>> For now, just moving this stuff to /lib would be helpful for other future user.
> I agree this code may need to be moved to /lib someday, but I wouldn't
> hurry with that.
> Right now it is quite KASAN-specific, and it's unclear yet whether
> anyone else is going to use it.
> I suggest we keep it in mm/kasan for now, and factor the common parts
> into /lib when the need arises.

Please consider it one more time. I really have a plan to use it on page owner,
because using page owner requires too many memory for stack trace and
it changes system behaviour a lot.

Page owner uses following structure to store stack trace.

struct page_ext {
        unsigned long flags;
#ifdef CONFIG_PAGE_OWNER
        unsigned int order;
        gfp_t gfp_mask;
        unsigned int nr_entries;
        int last_migrate_reason;
        unsigned long trace_entries[8];
#endif
};

Using stack depot in page owner would be straight forward if stack depot
is in /lib. It is possible to move it when needed but it requires moving
a file and it would not be desirable.

>> BTW, is there any performance number? I guess that it could affect
>> the performance.
> I've compared the performance of KASAN with SLAB allocator on a small
> synthetic benchmark in two modes: with stack depot enabled and with
> kasan_save_stack() unconditionally returning 0.
> In the former case 8% more time was spent in the kernel than in the latter case.
>
> If I am not mistaking, for SLUB allocator the bookkeeping (enabled
> with the slub_debug=UZ boot options) take only 1.5 time, so the
> difference is worth looking into (at least before we switch SLUB to
> stack depot).

Okay.

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web