During swapoff the frontswap_map was NULL-ified before calling
frontswap_invalidate_area(). However the frontswap_invalidate_area()
exits early if frontswap_map is NULL. Invalidate was never called during
swapoff.
This patch moves frontswap_map_set() in swapoff just after calling
frontswap_invalidate_area() so outside of locks
(swap_lock and swap_info_struct->lock). This shouldn't be a problem as
during swapon the frontswap_map_set() is called also outside of any
locks.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
mm/swapfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 3963fc2..3a4896b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1922,10 +1922,10 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
p->cluster_info = NULL;
p->flags = 0;
frontswap_map = frontswap_map_get(p);
- frontswap_map_set(p, NULL);
spin_unlock(&p->lock);
spin_unlock(&swap_lock);
frontswap_invalidate_area(type);
+ frontswap_map_set(p, NULL);
mutex_unlock(&swapon_mutex);
free_percpu(p->percpu_cluster);
p->percpu_cluster = NULL;
--
1.7.9.5
On Mon, Oct 07, 2013 at 05:25:41PM +0200, Krzysztof Kozlowski wrote:
> During swapoff the frontswap_map was NULL-ified before calling
> frontswap_invalidate_area(). However the frontswap_invalidate_area()
> exits early if frontswap_map is NULL. Invalidate was never called during
> swapoff.
>
> This patch moves frontswap_map_set() in swapoff just after calling
> frontswap_invalidate_area() so outside of locks
> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> during swapon the frontswap_map_set() is called also outside of any
> locks.
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
Nice catch!
Reviewed-by: Seth Jennings <[email protected]>
> ---
> mm/swapfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3963fc2..3a4896b 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1922,10 +1922,10 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> p->cluster_info = NULL;
> p->flags = 0;
> frontswap_map = frontswap_map_get(p);
> - frontswap_map_set(p, NULL);
> spin_unlock(&p->lock);
> spin_unlock(&swap_lock);
> frontswap_invalidate_area(type);
> + frontswap_map_set(p, NULL);
> mutex_unlock(&swapon_mutex);
> free_percpu(p->percpu_cluster);
> p->percpu_cluster = NULL;
> --
> 1.7.9.5
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <[email protected]> wrote:
> During swapoff the frontswap_map was NULL-ified before calling
> frontswap_invalidate_area(). However the frontswap_invalidate_area()
> exits early if frontswap_map is NULL. Invalidate was never called during
> swapoff.
>
> This patch moves frontswap_map_set() in swapoff just after calling
> frontswap_invalidate_area() so outside of locks
> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> during swapon the frontswap_map_set() is called also outside of any
> locks.
>
Ahem. So there's a bunch of code in __frontswap_invalidate_area()
which hasn't ever been executed and nobody noticed it. So perhaps that
code isn't actually needed?
More seriously, this patch looks like it enables code which hasn't been
used or tested before. How well tested was this?
Are there any runtime-visible effects from this change?
On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
> On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <[email protected]> wrote:
>
> > During swapoff the frontswap_map was NULL-ified before calling
> > frontswap_invalidate_area(). However the frontswap_invalidate_area()
> > exits early if frontswap_map is NULL. Invalidate was never called during
> > swapoff.
> >
> > This patch moves frontswap_map_set() in swapoff just after calling
> > frontswap_invalidate_area() so outside of locks
> > (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> > during swapon the frontswap_map_set() is called also outside of any
> > locks.
> >
>
> Ahem. So there's a bunch of code in __frontswap_invalidate_area()
> which hasn't ever been executed and nobody noticed it. So perhaps that
> code isn't actually needed?
>
> More seriously, this patch looks like it enables code which hasn't been
> used or tested before. How well tested was this?
>
> Are there any runtime-visible effects from this change?
I tested zswap on x86 and x86-64 and there was no difference. This is
good as there shouldn't be visible anything because swapoff is unusing
all pages anyway:
try_to_unuse(type, false, 0); /* force all pages to be unused */
I haven't tested other frontswap users.
Best regards,
Krzysztof Kozlowski
On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <[email protected]> wrote:
> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
> > On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <[email protected]> wrote:
> >
> > > During swapoff the frontswap_map was NULL-ified before calling
> > > frontswap_invalidate_area(). However the frontswap_invalidate_area()
> > > exits early if frontswap_map is NULL. Invalidate was never called during
> > > swapoff.
> > >
> > > This patch moves frontswap_map_set() in swapoff just after calling
> > > frontswap_invalidate_area() so outside of locks
> > > (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> > > during swapon the frontswap_map_set() is called also outside of any
> > > locks.
> > >
> >
> > Ahem. So there's a bunch of code in __frontswap_invalidate_area()
> > which hasn't ever been executed and nobody noticed it. So perhaps that
> > code isn't actually needed?
> >
> > More seriously, this patch looks like it enables code which hasn't been
> > used or tested before. How well tested was this?
> >
> > Are there any runtime-visible effects from this change?
>
> I tested zswap on x86 and x86-64 and there was no difference. This is
> good as there shouldn't be visible anything because swapoff is unusing
> all pages anyway:
> try_to_unuse(type, false, 0); /* force all pages to be unused */
>
> I haven't tested other frontswap users.
So is that code in __frontswap_invalidate_area() unneeded?
On 10/09/2013 04:08 AM, Andrew Morton wrote:
> On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <[email protected]> wrote:
>
>> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
>>> On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <[email protected]> wrote:
>>>
>>>> During swapoff the frontswap_map was NULL-ified before calling
>>>> frontswap_invalidate_area(). However the frontswap_invalidate_area()
>>>> exits early if frontswap_map is NULL. Invalidate was never called during
>>>> swapoff.
>>>>
>>>> This patch moves frontswap_map_set() in swapoff just after calling
>>>> frontswap_invalidate_area() so outside of locks
>>>> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
>>>> during swapon the frontswap_map_set() is called also outside of any
>>>> locks.
>>>>
>>>
>>> Ahem. So there's a bunch of code in __frontswap_invalidate_area()
>>> which hasn't ever been executed and nobody noticed it. So perhaps that
>>> code isn't actually needed?
>>>
>>> More seriously, this patch looks like it enables code which hasn't been
>>> used or tested before. How well tested was this?
>>>
>>> Are there any runtime-visible effects from this change?
>>
>> I tested zswap on x86 and x86-64 and there was no difference. This is
>> good as there shouldn't be visible anything because swapoff is unusing
>> all pages anyway:
>> try_to_unuse(type, false, 0); /* force all pages to be unused */
>>
>> I haven't tested other frontswap users.
>
> So is that code in __frontswap_invalidate_area() unneeded?
>
I don't think so, it's still needed otherwise there will be memory leak.
I'm afraid nobody noticed the memory leak here before, this patch can
fix it. Sorry for didn't pay enough attention but please keep
__frontswap_invalidate_area().
--
Regards,
-Bob
On Tue, Oct 08, 2013 at 01:08:53PM -0700, Andrew Morton wrote:
> On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <[email protected]> wrote:
>
> > On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
> > > On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <[email protected]> wrote:
> > >
> > > > During swapoff the frontswap_map was NULL-ified before calling
> > > > frontswap_invalidate_area(). However the frontswap_invalidate_area()
> > > > exits early if frontswap_map is NULL. Invalidate was never called during
> > > > swapoff.
> > > >
> > > > This patch moves frontswap_map_set() in swapoff just after calling
> > > > frontswap_invalidate_area() so outside of locks
> > > > (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> > > > during swapon the frontswap_map_set() is called also outside of any
> > > > locks.
> > > >
> > >
> > > Ahem. So there's a bunch of code in __frontswap_invalidate_area()
> > > which hasn't ever been executed and nobody noticed it. So perhaps that
> > > code isn't actually needed?
> > >
> > > More seriously, this patch looks like it enables code which hasn't been
> > > used or tested before. How well tested was this?
> > >
> > > Are there any runtime-visible effects from this change?
> >
> > I tested zswap on x86 and x86-64 and there was no difference. This is
> > good as there shouldn't be visible anything because swapoff is unusing
> > all pages anyway:
> > try_to_unuse(type, false, 0); /* force all pages to be unused */
> >
> > I haven't tested other frontswap users.
>
> So is that code in __frontswap_invalidate_area() unneeded?
Yes, to expand on what Bob said, __frontswap_invalidate_area() is still
needed to let any frontswap backend free per-swaptype resources.
__frontswap_invalidate_area() is _not_ for freeing structures associated
with individual swapped out pages since all of the pages should be
brought back into memory by try_to_unuse() before
__frontswap_invalidate_area() is called.
The reason we never noticed this for zswap is that zswap has no
dynamically allocated per-type resources. In the expected case,
where all of the pages have been drained from zswap,
zswap_frontswap_invalidate_area() is a no-op.
Seth
On 10/09/2013 10:40 PM, Seth Jennings wrote:
> On Tue, Oct 08, 2013 at 01:08:53PM -0700, Andrew Morton wrote:
>> On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <[email protected]> wrote:
>>
>>> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
>>>> On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <[email protected]> wrote:
>>>>
>>>>> During swapoff the frontswap_map was NULL-ified before calling
>>>>> frontswap_invalidate_area(). However the frontswap_invalidate_area()
>>>>> exits early if frontswap_map is NULL. Invalidate was never called during
>>>>> swapoff.
>>>>>
>>>>> This patch moves frontswap_map_set() in swapoff just after calling
>>>>> frontswap_invalidate_area() so outside of locks
>>>>> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
>>>>> during swapon the frontswap_map_set() is called also outside of any
>>>>> locks.
>>>>>
>>>>
>>>> Ahem. So there's a bunch of code in __frontswap_invalidate_area()
>>>> which hasn't ever been executed and nobody noticed it. So perhaps that
>>>> code isn't actually needed?
>>>>
>>>> More seriously, this patch looks like it enables code which hasn't been
>>>> used or tested before. How well tested was this?
>>>>
>>>> Are there any runtime-visible effects from this change?
>>>
>>> I tested zswap on x86 and x86-64 and there was no difference. This is
>>> good as there shouldn't be visible anything because swapoff is unusing
>>> all pages anyway:
>>> try_to_unuse(type, false, 0); /* force all pages to be unused */
>>>
>>> I haven't tested other frontswap users.
>>
>> So is that code in __frontswap_invalidate_area() unneeded?
>
> Yes, to expand on what Bob said, __frontswap_invalidate_area() is still
> needed to let any frontswap backend free per-swaptype resources.
>
> __frontswap_invalidate_area() is _not_ for freeing structures associated
> with individual swapped out pages since all of the pages should be
> brought back into memory by try_to_unuse() before
> __frontswap_invalidate_area() is called.
>
> The reason we never noticed this for zswap is that zswap has no
> dynamically allocated per-type resources. In the expected case,
> where all of the pages have been drained from zswap,
> zswap_frontswap_invalidate_area() is a no-op.
>
Not exactly, see the bug fix "mm/zswap: bugfix: memory leak when
re-swapon" from Weijie.
Zswap needs invalidate_area() also.
Thanks,
-Bob
On Thu, Oct 10, 2013 at 09:29:07AM +0800, Bob Liu wrote:
> On 10/09/2013 10:40 PM, Seth Jennings wrote:
> >
> > The reason we never noticed this for zswap is that zswap has no
> > dynamically allocated per-type resources. In the expected case,
> > where all of the pages have been drained from zswap,
> > zswap_frontswap_invalidate_area() is a no-op.
> >
>
> Not exactly, see the bug fix "mm/zswap: bugfix: memory leak when
> re-swapon" from Weijie.
> Zswap needs invalidate_area() also.
I remembered this patch as soon as I sent out this note. What I said
about zswap_frontswap_invalidate_area() being a no-op in the expected
case is true as of v3.12-rc4, but it shouldn't be :)
I sent a note to Andrew reminding him to pull in that patch.
Thanks,
Seth
Thanks, Seth
On Thu, Oct 10, 2013 at 10:26 AM, Seth Jennings
<[email protected]> wrote:
> On Thu, Oct 10, 2013 at 09:29:07AM +0800, Bob Liu wrote:
>> On 10/09/2013 10:40 PM, Seth Jennings wrote:
>> >
>> > The reason we never noticed this for zswap is that zswap has no
>> > dynamically allocated per-type resources. In the expected case,
>> > where all of the pages have been drained from zswap,
>> > zswap_frontswap_invalidate_area() is a no-op.
>> >
>>
>> Not exactly, see the bug fix "mm/zswap: bugfix: memory leak when
>> re-swapon" from Weijie.
>> Zswap needs invalidate_area() also.
>
> I remembered this patch as soon as I sent out this note. What I said
> about zswap_frontswap_invalidate_area() being a no-op in the expected
> case is true as of v3.12-rc4, but it shouldn't be :)
>
> I sent a note to Andrew reminding him to pull in that patch.
>
> Thanks,
> Seth
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
I am sorry to interrupt this topic, but I found an tiny issue near that:
we can not "set_blocksize(bdev, p->old_block_size);" at the end of swapoff()
because swap_info p may be reused by concurrent swapon called
I think we need to save the p->old_block_size in a local var and use it instead
to Krzysztof : would you please add this in your patch?
Thanks
On Fri, 2013-10-11 at 10:23 +0800, Weijie Yang wrote:
> I am sorry to interrupt this topic, but I found an tiny issue near that:
>
> we can not "set_blocksize(bdev, p->old_block_size);" at the end of swapoff()
> because swap_info p may be reused by concurrent swapon called
> I think we need to save the p->old_block_size in a local var and use it instead
I confirm the race here (I was able to trigger it on the same swap type).
> to Krzysztof : would you please add this in your patch?
> Thanks
I think it should be another patch as this is not related with
frontswap. I'll send new one and add you as Reported-by. Is it OK with
you?
Best regards,
Krzysztof
On Fri, Oct 11, 2013 at 5:25 PM, Krzysztof Kozlowski
<[email protected]> wrote:
> On Fri, 2013-10-11 at 10:23 +0800, Weijie Yang wrote:
>> I am sorry to interrupt this topic, but I found an tiny issue near that:
>>
>> we can not "set_blocksize(bdev, p->old_block_size);" at the end of swapoff()
>> because swap_info p may be reused by concurrent swapon called
>> I think we need to save the p->old_block_size in a local var and use it instead
> I confirm the race here (I was able to trigger it on the same swap type).
>
>
>> to Krzysztof : would you please add this in your patch?
>> Thanks
> I think it should be another patch as this is not related with
> frontswap. I'll send new one and add you as Reported-by. Is it OK with
> you?
All right.
>
> Best regards,
> Krzysztof
>