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

Groups > linux.kernel > #1740388 > unrolled thread

Re: [PATCH] mm, swap: Make VMA based swap readahead configurable

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] mm, swap: Make VMA based swap readahead configurable Minchan Kim <minchan@kernel.org> - 2017-09-27 07:10 +0200
    Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Michal Hocko <mhocko@kernel.org> - 2017-09-27 09:50 +0200
      Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Minchan Kim <minchan@kernel.org> - 2017-09-27 10:10 +0200
        Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Michal Hocko <mhocko@kernel.org> - 2017-09-27 10:40 +0200
          Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Minchan Kim <minchan@kernel.org> - 2017-09-27 15:20 +0200
            Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Michal Hocko <mhocko@kernel.org> - 2017-09-27 15:30 +0200
              Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Minchan Kim <minchan@kernel.org> - 2017-09-27 15:50 +0200
                Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Michal Hocko <mhocko@kernel.org> - 2017-09-27 16:00 +0200
                  Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Minchan Kim <minchan@kernel.org> - 2017-09-27 16:20 +0200
                    Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Michal Hocko <mhocko@kernel.org> - 2017-09-27 16:20 +0200
                      Re: [PATCH] mm, swap: Make VMA based swap readahead configurable "Huang\, Ying" <ying.huang@intel.com> - 2017-09-28 03:10 +0200
                        Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Michal Hocko <mhocko@kernel.org> - 2017-10-02 09:30 +0200
                        Re: [PATCH] mm, swap: Make VMA based swap readahead configurable Dave Hansen <dave.hansen@intel.com> - 2017-10-02 22:50 +0200

#1740388 — Re: [PATCH] mm, swap: Make VMA based swap readahead configurable

On Tue, Sep 26, 2017 at 03:21:29PM +0200, Michal Hocko wrote:
> On Thu 21-09-17 09:33:10, Huang, Ying wrote:
> > From: Huang Ying <ying.huang@intel.com>
> > 
> > This patch adds a new Kconfig option VMA_SWAP_READAHEAD and wraps VMA
> > based swap readahead code inside #ifdef CONFIG_VMA_SWAP_READAHEAD/#endif.
> > This is more friendly for tiny kernels.
> 
> How (much)?
> 
> > And as pointed to by Minchan
> > Kim, give people who want to disable the swap readahead an opportunity
> > to notice the changes to the swap readahead algorithm and the
> > corresponding knobs.
> 
> Why would anyone want that?
> 
> Please note that adding new config options make the already complicated
> config space even more problematic so there should be a good reason to
> add one. Please make sure your justification is clear on why this is
> worth the future maintenance and configurability burden.

The problem is users have disabled swap readahead by echo 0 > /proc/sys/
vm/page-cluster are regressed by this new interface /sys/kernel/mm/swap/
vma_ra_max_order. Because for disabling readahead completely, they should
disable vma_ra_max_order as well as page-cluster from now on.

So, goal of new config to notice new feature to admins so they can be aware
of new konb vma_ra_max_order as well as page-cluster.
I canont think other better idea to preventing such regression.

http://lkml.kernel.org/r/%3C20170913014019.GB29422@bbox%3E

[toc] | [next] | [standalone]

#1740472

On Wed 27-09-17 14:04:01, Minchan Kim wrote:
> On Tue, Sep 26, 2017 at 03:21:29PM +0200, Michal Hocko wrote:
> > On Thu 21-09-17 09:33:10, Huang, Ying wrote:
> > > From: Huang Ying <ying.huang@intel.com>
> > > 
> > > This patch adds a new Kconfig option VMA_SWAP_READAHEAD and wraps VMA
> > > based swap readahead code inside #ifdef CONFIG_VMA_SWAP_READAHEAD/#endif.
> > > This is more friendly for tiny kernels.
> > 
> > How (much)?
> > 
> > > And as pointed to by Minchan
> > > Kim, give people who want to disable the swap readahead an opportunity
> > > to notice the changes to the swap readahead algorithm and the
> > > corresponding knobs.
> > 
> > Why would anyone want that?
> > 
> > Please note that adding new config options make the already complicated
> > config space even more problematic so there should be a good reason to
> > add one. Please make sure your justification is clear on why this is
> > worth the future maintenance and configurability burden.
> 
> The problem is users have disabled swap readahead by echo 0 > /proc/sys/
> vm/page-cluster are regressed by this new interface /sys/kernel/mm/swap/
> vma_ra_max_order. Because for disabling readahead completely, they should
> disable vma_ra_max_order as well as page-cluster from now on.
> 
> So, goal of new config to notice new feature to admins so they can be aware
> of new konb vma_ra_max_order as well as page-cluster.
> I canont think other better idea to preventing such regression.
> 
> http://lkml.kernel.org/r/%3C20170913014019.GB29422@bbox%3E

So, how are you going to configure this when you do not know whether
zram will be used? In other words what should e.g. distribution set this
to?

-- 
Michal Hocko
SUSE Labs

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

#1740479

On Wed, Sep 27, 2017 at 09:48:35AM +0200, Michal Hocko wrote:
> On Wed 27-09-17 14:04:01, Minchan Kim wrote:
> > On Tue, Sep 26, 2017 at 03:21:29PM +0200, Michal Hocko wrote:
> > > On Thu 21-09-17 09:33:10, Huang, Ying wrote:
> > > > From: Huang Ying <ying.huang@intel.com>
> > > > 
> > > > This patch adds a new Kconfig option VMA_SWAP_READAHEAD and wraps VMA
> > > > based swap readahead code inside #ifdef CONFIG_VMA_SWAP_READAHEAD/#endif.
> > > > This is more friendly for tiny kernels.
> > > 
> > > How (much)?
> > > 
> > > > And as pointed to by Minchan
> > > > Kim, give people who want to disable the swap readahead an opportunity
> > > > to notice the changes to the swap readahead algorithm and the
> > > > corresponding knobs.
> > > 
> > > Why would anyone want that?
> > > 
> > > Please note that adding new config options make the already complicated
> > > config space even more problematic so there should be a good reason to
> > > add one. Please make sure your justification is clear on why this is
> > > worth the future maintenance and configurability burden.
> > 
> > The problem is users have disabled swap readahead by echo 0 > /proc/sys/
> > vm/page-cluster are regressed by this new interface /sys/kernel/mm/swap/
> > vma_ra_max_order. Because for disabling readahead completely, they should
> > disable vma_ra_max_order as well as page-cluster from now on.
> > 
> > So, goal of new config to notice new feature to admins so they can be aware
> > of new konb vma_ra_max_order as well as page-cluster.
> > I canont think other better idea to preventing such regression.
> > 
> > http://lkml.kernel.org/r/%3C20170913014019.GB29422@bbox%3E
> 
> So, how are you going to configure this when you do not know whether
> zram will be used? In other words what should e.g. distribution set this
> to?

I have no idea. Unfortunately, it depends on them. If they want to use
zram as swap, they should fix the script. Surely, I don't like it.
Instead, I wanted that page-cluster zeroing disables both virtual/pysical
swap readahead not to break current userspace. However, Huang doesn't
liek it.
If you have better idea, please suggest.

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

#1740500

On Wed 27-09-17 17:04:32, Minchan Kim wrote:
> On Wed, Sep 27, 2017 at 09:48:35AM +0200, Michal Hocko wrote:
> > On Wed 27-09-17 14:04:01, Minchan Kim wrote:
[...]
> > > The problem is users have disabled swap readahead by echo 0 > /proc/sys/
> > > vm/page-cluster are regressed by this new interface /sys/kernel/mm/swap/
> > > vma_ra_max_order. Because for disabling readahead completely, they should
> > > disable vma_ra_max_order as well as page-cluster from now on.
> > > 
> > > So, goal of new config to notice new feature to admins so they can be aware
> > > of new konb vma_ra_max_order as well as page-cluster.
> > > I canont think other better idea to preventing such regression.
> > > 
> > > http://lkml.kernel.org/r/%3C20170913014019.GB29422@bbox%3E
> > 
> > So, how are you going to configure this when you do not know whether
> > zram will be used? In other words what should e.g. distribution set this
> > to?
> 
> I have no idea. Unfortunately, it depends on them. If they want to use
> zram as swap, they should fix the script. Surely, I don't like it.
> Instead, I wanted that page-cluster zeroing disables both virtual/pysical
> swap readahead not to break current userspace. However, Huang doesn't
> liek it.
> If you have better idea, please suggest.

I understand your frustration but config options are not there to bypass
proper design decisions. Why cannot we unconditionally disable all the
read ahead when zram is enabled?
-- 
Michal Hocko
SUSE Labs

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

#1740710

On Wed, Sep 27, 2017 at 10:35:12AM +0200, Michal Hocko wrote:
> On Wed 27-09-17 17:04:32, Minchan Kim wrote:
> > On Wed, Sep 27, 2017 at 09:48:35AM +0200, Michal Hocko wrote:
> > > On Wed 27-09-17 14:04:01, Minchan Kim wrote:
> [...]
> > > > The problem is users have disabled swap readahead by echo 0 > /proc/sys/
> > > > vm/page-cluster are regressed by this new interface /sys/kernel/mm/swap/
> > > > vma_ra_max_order. Because for disabling readahead completely, they should
> > > > disable vma_ra_max_order as well as page-cluster from now on.
> > > > 
> > > > So, goal of new config to notice new feature to admins so they can be aware
> > > > of new konb vma_ra_max_order as well as page-cluster.
> > > > I canont think other better idea to preventing such regression.
> > > > 
> > > > http://lkml.kernel.org/r/%3C20170913014019.GB29422@bbox%3E
> > > 
> > > So, how are you going to configure this when you do not know whether
> > > zram will be used? In other words what should e.g. distribution set this
> > > to?
> > 
> > I have no idea. Unfortunately, it depends on them. If they want to use
> > zram as swap, they should fix the script. Surely, I don't like it.
> > Instead, I wanted that page-cluster zeroing disables both virtual/pysical
> > swap readahead not to break current userspace. However, Huang doesn't
> > liek it.
> > If you have better idea, please suggest.
> 
> I understand your frustration but config options are not there to bypass
> proper design decisions. Why cannot we unconditionally disable all the
> read ahead when zram is enabled?

It's not a zram specific issue. Every users who have disabled swap readahead
via page-cluster will be broken, too.

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

#1740711

On Wed 27-09-17 22:15:11, Minchan Kim wrote:
> On Wed, Sep 27, 2017 at 10:35:12AM +0200, Michal Hocko wrote:
> > On Wed 27-09-17 17:04:32, Minchan Kim wrote:
> > > On Wed, Sep 27, 2017 at 09:48:35AM +0200, Michal Hocko wrote:
> > > > On Wed 27-09-17 14:04:01, Minchan Kim wrote:
> > [...]
> > > > > The problem is users have disabled swap readahead by echo 0 > /proc/sys/
> > > > > vm/page-cluster are regressed by this new interface /sys/kernel/mm/swap/
> > > > > vma_ra_max_order. Because for disabling readahead completely, they should
> > > > > disable vma_ra_max_order as well as page-cluster from now on.
> > > > > 
> > > > > So, goal of new config to notice new feature to admins so they can be aware
> > > > > of new konb vma_ra_max_order as well as page-cluster.
> > > > > I canont think other better idea to preventing such regression.
> > > > > 
> > > > > http://lkml.kernel.org/r/%3C20170913014019.GB29422@bbox%3E
> > > > 
> > > > So, how are you going to configure this when you do not know whether
> > > > zram will be used? In other words what should e.g. distribution set this
> > > > to?
> > > 
> > > I have no idea. Unfortunately, it depends on them. If they want to use
> > > zram as swap, they should fix the script. Surely, I don't like it.
> > > Instead, I wanted that page-cluster zeroing disables both virtual/pysical
> > > swap readahead not to break current userspace. However, Huang doesn't
> > > liek it.
> > > If you have better idea, please suggest.
> > 
> > I understand your frustration but config options are not there to bypass
> > proper design decisions. Why cannot we unconditionally disable all the
> > read ahead when zram is enabled?
> 
> It's not a zram specific issue. Every users who have disabled swap readahead
> via page-cluster will be broken, too.

Do you have any examples outside of zram? Also I do not see why we
simply cannot disable swap readahead when page-cluster is 0?
-- 
Michal Hocko
SUSE Labs

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

#1740742

On Wed, Sep 27, 2017 at 03:22:41PM +0200, Michal Hocko wrote:
> On Wed 27-09-17 22:15:11, Minchan Kim wrote:
> > On Wed, Sep 27, 2017 at 10:35:12AM +0200, Michal Hocko wrote:
> > > On Wed 27-09-17 17:04:32, Minchan Kim wrote:
> > > > On Wed, Sep 27, 2017 at 09:48:35AM +0200, Michal Hocko wrote:
> > > > > On Wed 27-09-17 14:04:01, Minchan Kim wrote:
> > > [...]
> > > > > > The problem is users have disabled swap readahead by echo 0 > /proc/sys/
> > > > > > vm/page-cluster are regressed by this new interface /sys/kernel/mm/swap/
> > > > > > vma_ra_max_order. Because for disabling readahead completely, they should
> > > > > > disable vma_ra_max_order as well as page-cluster from now on.
> > > > > > 
> > > > > > So, goal of new config to notice new feature to admins so they can be aware
> > > > > > of new konb vma_ra_max_order as well as page-cluster.
> > > > > > I canont think other better idea to preventing such regression.
> > > > > > 
> > > > > > http://lkml.kernel.org/r/%3C20170913014019.GB29422@bbox%3E
> > > > > 
> > > > > So, how are you going to configure this when you do not know whether
> > > > > zram will be used? In other words what should e.g. distribution set this
> > > > > to?
> > > > 
> > > > I have no idea. Unfortunately, it depends on them. If they want to use
> > > > zram as swap, they should fix the script. Surely, I don't like it.
> > > > Instead, I wanted that page-cluster zeroing disables both virtual/pysical
> > > > swap readahead not to break current userspace. However, Huang doesn't
> > > > liek it.
> > > > If you have better idea, please suggest.
> > > 
> > > I understand your frustration but config options are not there to bypass
> > > proper design decisions. Why cannot we unconditionally disable all the
> > > read ahead when zram is enabled?
> > 
> > It's not a zram specific issue. Every users who have disabled swap readahead
> > via page-cluster will be broken, too.
> 
> Do you have any examples outside of zram? Also I do not see why we

I'm not a god to know every usecases on earth. It's just knob and
it's have been there for a long time with following semantic:

        Zero disables swap readahead completely.

So, anyuser can use it by their reasons(e.g., small memory system)

I don't want to play with pointless game "Hey, give me an example.
If you couldn't, no worth to keep the semantic" on such long time
simple/clear semantic.


> simply cannot disable swap readahead when page-cluster is 0?

That's was what I want really but Huang want to use two readahead
algorithms in parallel so he wanted to keep two separated disable
knobs.


> -- 
> Michal Hocko
> SUSE Labs

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

#1740752

On Wed 27-09-17 22:41:17, Minchan Kim wrote:
> On Wed, Sep 27, 2017 at 03:22:41PM +0200, Michal Hocko wrote:
[...]
> > simply cannot disable swap readahead when page-cluster is 0?
> 
> That's was what I want really but Huang want to use two readahead
> algorithms in parallel so he wanted to keep two separated disable
> knobs.

If it breaks existing and documented behavior then it is a clear
regression and it should be fixed. I do not see why this should be
disputable at all.

Working around an issue with a config option sounds like the wrong way
to go because those who cannot do that unconditionally would still see a
regression.
-- 
Michal Hocko
SUSE Labs

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

#1740769

On Wed, Sep 27, 2017 at 03:50:34PM +0200, Michal Hocko wrote:
> On Wed 27-09-17 22:41:17, Minchan Kim wrote:
> > On Wed, Sep 27, 2017 at 03:22:41PM +0200, Michal Hocko wrote:
> [...]
> > > simply cannot disable swap readahead when page-cluster is 0?
> > 
> > That's was what I want really but Huang want to use two readahead
> > algorithms in parallel so he wanted to keep two separated disable
> > knobs.
> 
> If it breaks existing and documented behavior then it is a clear
> regression and it should be fixed. I do not see why this should be
> disputable at all.

Indeed but Huang doesn't think so. He has thought it's not a regression.
Frankly speaking, I'm really bored of discussing with it.
https://marc.info/?l=linux-mm&m=150526413319763&w=2

So I passed the decision to Andrew.
http://lkml.kernel.org/r/<20170913014019.GB29422@bbox>

The config option idea is compromise approach although I don't like it
and still believe it's simple clear *regression* so 0 page-cluster
should keep the swap readahead disabled.

> 
> Working around an issue with a config option sounds like the wrong way
> to go because those who cannot do that unconditionally would still see a
> regression.

I absolutely agree but as I said, the discussion was not productive
even though I did best effort to persuade. That's all for my side as
contributor/reviewer. Decision is up to maintainer. ;-)

Thanks for the opinion, Michal.

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

#1740772

On Wed 27-09-17 23:10:08, Minchan Kim wrote:
> On Wed, Sep 27, 2017 at 03:50:34PM +0200, Michal Hocko wrote:
> > On Wed 27-09-17 22:41:17, Minchan Kim wrote:
> > > On Wed, Sep 27, 2017 at 03:22:41PM +0200, Michal Hocko wrote:
> > [...]
> > > > simply cannot disable swap readahead when page-cluster is 0?
> > > 
> > > That's was what I want really but Huang want to use two readahead
> > > algorithms in parallel so he wanted to keep two separated disable
> > > knobs.
> > 
> > If it breaks existing and documented behavior then it is a clear
> > regression and it should be fixed. I do not see why this should be
> > disputable at all.
> 
> Indeed but Huang doesn't think so. He has thought it's not a regression.
> Frankly speaking, I'm really bored of discussing with it.
> https://marc.info/?l=linux-mm&m=150526413319763&w=2

Then send a patch explaining why you consider this a regression with
some numbers backing it and I will happily ack it.

> So I passed the decision to Andrew.
> http://lkml.kernel.org/r/<20170913014019.GB29422@bbox>
> 
> The config option idea is compromise approach although I don't like it
> and still believe it's simple clear *regression* so 0 page-cluster
> should keep the swap readahead disabled.

It is not a compromise. The regression is still there for many users
potentially (just consider zram distribution kernel users...).
-- 
Michal Hocko
SUSE Labs

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

#1741122

Hi, Michal,

Michal Hocko <mhocko@kernel.org> writes:

> On Wed 27-09-17 23:10:08, Minchan Kim wrote:
>> On Wed, Sep 27, 2017 at 03:50:34PM +0200, Michal Hocko wrote:
>> > On Wed 27-09-17 22:41:17, Minchan Kim wrote:
>> > > On Wed, Sep 27, 2017 at 03:22:41PM +0200, Michal Hocko wrote:
>> > [...]
>> > > > simply cannot disable swap readahead when page-cluster is 0?
>> > > 
>> > > That's was what I want really but Huang want to use two readahead
>> > > algorithms in parallel so he wanted to keep two separated disable
>> > > knobs.
>> > 
>> > If it breaks existing and documented behavior then it is a clear
>> > regression and it should be fixed. I do not see why this should be
>> > disputable at all.
>> 
>> Indeed but Huang doesn't think so. He has thought it's not a regression.
>> Frankly speaking, I'm really bored of discussing with it.
>> https://marc.info/?l=linux-mm&m=150526413319763&w=2
>
> Then send a patch explaining why you consider this a regression with
> some numbers backing it and I will happily ack it.

I still think there may be a performance regression for some users
because of the change of the algorithm and the knobs, and the
performance regression can be resolved via setting the new knob.  But I
don't think there will be a functionality regression.  Do you agree?

Best Regards,
Huang, Ying

>> So I passed the decision to Andrew.
>> http://lkml.kernel.org/r/<20170913014019.GB29422@bbox>
>> 
>> The config option idea is compromise approach although I don't like it
>> and still believe it's simple clear *regression* so 0 page-cluster
>> should keep the swap readahead disabled.
>
> It is not a compromise. The regression is still there for many users
> potentially (just consider zram distribution kernel users...).

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

#1742988

On Thu 28-09-17 09:02:20, Huang, Ying wrote:
> Hi, Michal,
> 
> Michal Hocko <mhocko@kernel.org> writes:
> 
> > On Wed 27-09-17 23:10:08, Minchan Kim wrote:
> >> On Wed, Sep 27, 2017 at 03:50:34PM +0200, Michal Hocko wrote:
> >> > On Wed 27-09-17 22:41:17, Minchan Kim wrote:
> >> > > On Wed, Sep 27, 2017 at 03:22:41PM +0200, Michal Hocko wrote:
> >> > [...]
> >> > > > simply cannot disable swap readahead when page-cluster is 0?
> >> > > 
> >> > > That's was what I want really but Huang want to use two readahead
> >> > > algorithms in parallel so he wanted to keep two separated disable
> >> > > knobs.
> >> > 
> >> > If it breaks existing and documented behavior then it is a clear
> >> > regression and it should be fixed. I do not see why this should be
> >> > disputable at all.
> >> 
> >> Indeed but Huang doesn't think so. He has thought it's not a regression.
> >> Frankly speaking, I'm really bored of discussing with it.
> >> https://marc.info/?l=linux-mm&m=150526413319763&w=2
> >
> > Then send a patch explaining why you consider this a regression with
> > some numbers backing it and I will happily ack it.
> 
> I still think there may be a performance regression for some users
> because of the change of the algorithm and the knobs, and the
> performance regression can be resolved via setting the new knob.  But I
> don't think there will be a functionality regression.  Do you agree?

I am not sure I understand. One thing is clear though. Your change has
introduced a regression as described by Minchan. And that has to be
resolved no matter what. You cannot expect users will tweak the system
to resolve it or configure their systems in a specific way.

-- 
Michal Hocko
SUSE Labs

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

#1743318

On 09/27/2017 06:02 PM, Huang, Ying wrote:
> I still think there may be a performance regression for some users
> because of the change of the algorithm and the knobs, and the
> performance regression can be resolved via setting the new knob.  But I
> don't think there will be a functionality regression.  Do you agree?

A performance regression is a regression.  I don't understand why we are
splitting hairs as to what kind of regression it is.

Are you only willing to fix it if it's a functional regression?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web