2014-10-07 22:04:21

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH 5/5] mm: poison page struct

On 09/29/2014 06:47 PM, Sasha Levin wrote:
> struct page {
> +#ifdef CONFIG_DEBUG_VM_POISON
> + u32 poison_start;
> +#endif
> /* First double word block */
> unsigned long flags; /* Atomic flags, some possibly
> * updated asynchronously */
> @@ -196,6 +199,9 @@ struct page {
> #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
> int _last_cpupid;
> #endif
> +#ifdef CONFIG_DEBUG_VM_POISON
> + u32 poison_end;
> +#endif
> }

Does this break slub's __cmpxchg_double_slab trick? I thought it
required page->freelist and page->counters to be doubleword-aligned.

It's not like we really require this optimization when we're debugging,
but trying to use it will unnecessarily slow things down.

FWIW, if you're looking to trim down the number of lines of code, you
could certainly play some macro tricks and #ifdef tricks.

struct vm_poison {
#ifdef CONFIG_DEBUG_VM_POISON
u32 val;
#endif
};

Then, instead of #ifdefs in each structure, you do:

struct page {
struct vm_poison poison_start;
... other gunk
struct vm_poison poison_end;
};


Subject: Re: [PATCH 5/5] mm: poison page struct

On Tue, 7 Oct 2014, Dave Hansen wrote:

> Does this break slub's __cmpxchg_double_slab trick? I thought it
> required page->freelist and page->counters to be doubleword-aligned.

Sure that would be required for it to work.

> It's not like we really require this optimization when we're debugging,
> but trying to use it will unnecessarily slow things down.

Debugging by inserting more data into the page struct will already cause a
significant slow down because the cache footprint of key functions will
increase significantly. I would think that using the fallback functions
is reasonable in this scenario,

2014-10-08 14:23:18

by Sasha Levin

[permalink] [raw]
Subject: Re: [PATCH 5/5] mm: poison page struct

On 10/07/2014 06:02 PM, Dave Hansen wrote:
> On 09/29/2014 06:47 PM, Sasha Levin wrote:
>> struct page {
>> +#ifdef CONFIG_DEBUG_VM_POISON
>> + u32 poison_start;
>> +#endif
>> /* First double word block */
>> unsigned long flags; /* Atomic flags, some possibly
>> * updated asynchronously */
>> @@ -196,6 +199,9 @@ struct page {
>> #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
>> int _last_cpupid;
>> #endif
>> +#ifdef CONFIG_DEBUG_VM_POISON
>> + u32 poison_end;
>> +#endif
>> }
>
> Does this break slub's __cmpxchg_double_slab trick? I thought it
> required page->freelist and page->counters to be doubleword-aligned.

I'll probably have to switch it to 8 bytes anyways to make it work with
kasan. This should take care of the slub optimization as well.

> It's not like we really require this optimization when we're debugging,
> but trying to use it will unnecessarily slow things down.
>
> FWIW, if you're looking to trim down the number of lines of code, you
> could certainly play some macro tricks and #ifdef tricks.
>
> struct vm_poison {
> #ifdef CONFIG_DEBUG_VM_POISON
> u32 val;
> #endif
> };
>
> Then, instead of #ifdefs in each structure, you do:
>
> struct page {
> struct vm_poison poison_start;
> ... other gunk
> struct vm_poison poison_end;
> };

Agreed, I'll reword that in the next version.


Thanks,
Sasha