Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Fri, Oct 11, 2013 at 5:25 PM, Krzysztof Kozlowski 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Fri, Oct 11, 2013 at 5:25 PM, Krzysztof Kozlowski k.kozlow...@samsung.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
Thanks, Seth On Thu, Oct 10, 2013 at 10:26 AM, Seth Jennings 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 majord...@vger.kernel.org > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
Thanks, Seth On Thu, Oct 10, 2013 at 10:26 AM, Seth Jennings sjenni...@variantweb.net 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 majord...@vger.kernel.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 >> wrote: >> >>> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: On Mon, 07 Oct 2013 17:25:41 +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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Tue, Oct 08, 2013 at 01:08:53PM -0700, Andrew Morton wrote: > On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski > wrote: > > > On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: > > > On Mon, 07 Oct 2013 17:25:41 +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. > > > > > > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On 10/09/2013 04:08 AM, Andrew Morton wrote: > On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski > wrote: > >> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: >>> On Mon, 07 Oct 2013 17:25:41 +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. >>> >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On 10/09/2013 04:08 AM, Andrew Morton wrote: On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com wrote: On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Tue, Oct 08, 2013 at 01:08:53PM -0700, Andrew Morton wrote: On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com wrote: On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 k.kozlow...@samsung.com wrote: On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski wrote: > On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: > > On Mon, 07 Oct 2013 17:25:41 +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. > > > > > > > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: > On Mon, 07 Oct 2013 17:25:41 +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. > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com wrote: On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote: On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com 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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Mon, 07 Oct 2013 17:25:41 +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. > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 Nice catch! Reviewed-by: Seth Jennings > --- > 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(>lock); > spin_unlock(_lock); > frontswap_invalidate_area(type); > + frontswap_map_set(p, NULL); > mutex_unlock(_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 majord...@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: mailto:"d...@kvack.org;> em...@kvack.org > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] frontswap: enable call to invalidate area on swapoff
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 --- 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(>lock); spin_unlock(_lock); frontswap_invalidate_area(type); + frontswap_map_set(p, NULL); mutex_unlock(_mutex); free_percpu(p->percpu_cluster); p->percpu_cluster = NULL; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] frontswap: enable call to invalidate area on swapoff
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 k.kozlow...@samsung.com --- 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 from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
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 k.kozlow...@samsung.com Nice catch! Reviewed-by: Seth Jennings sjenn...@linux.vnet.ibm.com --- 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 majord...@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] frontswap: enable call to invalidate area on swapoff
On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski k.kozlow...@samsung.com 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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
