mm/swap: add function get_total_swap_pages to expose total_swap_pages

Commit Message

He, Hongbo Jan. 29, 2018, 8:29 a.m. UTC

ttm module needs it to determine its internal parameter setting.

Signed-off-by: Roger He <[e-mail]>
---
 include/linux/swap.h |  6 ++++++
 mm/swapfile.c        | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

Comments

Michal Hocko Jan. 29, 2018, 4:31 p.m. UTC | #1

On Mon 29-01-18 16:29:42, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.

Could you be more specific why?

> Signed-off-by: Roger He <[e-mail]>
> ---
>  include/linux/swap.h |  6 ++++++
>  mm/swapfile.c        | 15 +++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index c2b8128..708d66f 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>  struct backing_dev_info;
>  extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
>  extern void exit_swap_address_space(unsigned int type);
> +extern long get_total_swap_pages(void);
>  
>  #else /* CONFIG_SWAP */
>  
> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)
>  {
>  }
>  
> +long get_total_swap_pages(void)
> +{
> +	return 0;
> +}
> +
>  #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
>  #define swapcache_prepare(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
>  
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02..a0062eb 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>  
>  atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>  
> +/*
> + * expose this value for others use
> + */
> +long get_total_swap_pages(void)
> +{
> +	long ret;
> +
> +	spin_lock(&swap_lock);
> +	ret = total_swap_pages;
> +	spin_unlock(&swap_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
> +
>  static inline unsigned char swap_count(unsigned char ent)
>  {
>  	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */
> -- 
> 2.7.4

He, Hongbo Jan. 30, 2018, 2:56 a.m. UTC | #2

Hi Michal:

We need a API to tell TTM module the system totally has how many swap cache.
Then TTM module can use it to restrict how many the swap cache it can use to prevent triggering OOM.
For Now we set the threshold of swap size TTM used as 1/2 * total size and leave the rest for others use.

But get_nr_swap_pages is the only API we can accessed from other module now.
It can't cover the case of the dynamic swap size increment.
I mean: user can use "swapon" to enable new swap file or swap disk dynamically or "swapoff" to disable swap space.

Thanks
Roger(Hongbo.He)

-----Original Message-----
From: dri-devel [mailto:[e-mail]] On Behalf Of Michal Hocko

Sent: Tuesday, January 30, 2018 12:31 AM
To: He, Roger <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]; Koenig, Christian <[e-mail]>
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

On Mon 29-01-18 16:29:42, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.


Could you be more specific why?

> Signed-off-by: Roger He <[e-mail]>

> ---

>  include/linux/swap.h |  6 ++++++

>  mm/swapfile.c        | 15 +++++++++++++++

>  2 files changed, 21 insertions(+)

> 

> diff --git a/include/linux/swap.h b/include/linux/swap.h index 

> c2b8128..708d66f 100644

> --- a/include/linux/swap.h

> +++ b/include/linux/swap.h

> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);  

> struct backing_dev_info;  extern int init_swap_address_space(unsigned 

> int type, unsigned long nr_pages);  extern void 

> exit_swap_address_space(unsigned int type);

> +extern long get_total_swap_pages(void);

>  

>  #else /* CONFIG_SWAP */

>  

> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)  {  

> }

>  

> +long get_total_swap_pages(void)

> +{

> +	return 0;

> +}

> +

>  #define free_swap_and_cache(e) ({(is_migration_entry(e) || 

> is_device_private_entry(e));})  #define swapcache_prepare(e) 

> ({(is_migration_entry(e) || is_device_private_entry(e));})

>  

> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb 

> 100644

> --- a/mm/swapfile.c

> +++ b/mm/swapfile.c

> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>  

>  atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>  

> +/*

> + * expose this value for others use

> + */

> +long get_total_swap_pages(void)

> +{

> +	long ret;

> +

> +	spin_lock(&swap_lock);

> +	ret = total_swap_pages;

> +	spin_unlock(&swap_lock);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

> +

>  static inline unsigned char swap_count(unsigned char ent)  {

>  	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

> --

> 2.7.4


--
Michal Hocko
SUSE Labs
_______________________________________________
dri-devel mailing list
[e-mail]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

He, Hongbo Jan. 30, 2018, 5:13 a.m. UTC | #3

get_nr_swap_pages is the only API we can accessed from other module now.
	It can't cover the case of the dynamic swap size increment.
	I mean: user can use "swapon" to enable new swap file or swap disk dynamically or "swapoff" to disable swap space.

Above is why we always to get swap cache size rather than getting it once at module initialization time.
That is internal in TTM. Please ignore that.

And why TTM needs get_total_swap_pages instead of using get_nr_swap_pages directly. That because
even though the TTM buffer has been swapped out, at the start they also stay in system memory by shmem. Later at some point when
Under high memory pressure, Those buffers all are flushed into swap disk and used more swap disk size or even use up all swap size. That is not what we want and still has random OOM. So we need a API to get total swap size and control the swap size used by TTM very accurately.

Thanks
Roger(Hongbo.He)
-----Original Message-----
From: dri-devel [mailto:[e-mail]] On Behalf Of He, Roger

Sent: Tuesday, January 30, 2018 10:57 AM
To: Michal Hocko <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]; Koenig, Christian <[e-mail]>
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Hi Michal:

We need a API to tell TTM module the system totally has how many swap cache.
Then TTM module can use it to restrict how many the swap cache it can use to prevent triggering OOM.
For Now we set the threshold of swap size TTM used as 1/2 * total size and leave the rest for others use.


get_nr_swap_pages is the only API we can accessed from other module now.
	It can't cover the case of the dynamic swap size increment.
	I mean: user can use "swapon" to enable new swap file or swap disk dynamically or "swapoff" to disable swap space.

Thanks
Roger(Hongbo.He)

-----Original Message-----
From: dri-devel [mailto:[e-mail]] On Behalf Of Michal Hocko

Sent: Tuesday, January 30, 2018 12:31 AM
To: He, Roger <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]; Koenig, Christian <[e-mail]>
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

On Mon 29-01-18 16:29:42, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.


Could you be more specific why?

> Signed-off-by: Roger He <[e-mail]>

> ---

>  include/linux/swap.h |  6 ++++++

>  mm/swapfile.c        | 15 +++++++++++++++

>  2 files changed, 21 insertions(+)

> 

> diff --git a/include/linux/swap.h b/include/linux/swap.h index 

> c2b8128..708d66f 100644

> --- a/include/linux/swap.h

> +++ b/include/linux/swap.h

> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *); struct 

> backing_dev_info;  extern int init_swap_address_space(unsigned int 

> type, unsigned long nr_pages);  extern void 

> exit_swap_address_space(unsigned int type);

> +extern long get_total_swap_pages(void);

>  

>  #else /* CONFIG_SWAP */

>  

> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)  { 

> }

>  

> +long get_total_swap_pages(void)

> +{

> +	return 0;

> +}

> +

>  #define free_swap_and_cache(e) ({(is_migration_entry(e) ||

> is_device_private_entry(e));})  #define swapcache_prepare(e)

> ({(is_migration_entry(e) || is_device_private_entry(e));})

>  

> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb

> 100644

> --- a/mm/swapfile.c

> +++ b/mm/swapfile.c

> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>  

>  atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>  

> +/*

> + * expose this value for others use

> + */

> +long get_total_swap_pages(void)

> +{

> +	long ret;

> +

> +	spin_lock(&swap_lock);

> +	ret = total_swap_pages;

> +	spin_unlock(&swap_lock);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

> +

>  static inline unsigned char swap_count(unsigned char ent)  {

>  	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

> --

> 2.7.4


--
Michal Hocko
SUSE Labs
_______________________________________________
dri-devel mailing list
[e-mail]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[e-mail]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Michal Hocko Jan. 30, 2018, 7:55 a.m. UTC | #4

On Tue 30-01-18 02:56:51, He, Roger wrote:
> Hi Michal:
> 
> We need a API to tell TTM module the system totally has how many swap
> cache.  Then TTM module can use it to restrict how many the swap cache
> it can use to prevent triggering OOM.  For Now we set the threshold of
> swap size TTM used as 1/2 * total size and leave the rest for others
> use.

Why do you so much memory? Are you going to use TB of memory on large
systems? What about memory hotplug when the memory is added/released?
 
> But get_nr_swap_pages is the only API we can accessed from other
> module now.  It can't cover the case of the dynamic swap size
> increment.  I mean: user can use "swapon" to enable new swap file or
> swap disk dynamically or "swapoff" to disable swap space.

Exactly. Your scaling configuration based on get_nr_swap_pages or the
available memory simply sounds wrong.

Christian König Jan. 30, 2018, 9 a.m. UTC | #5

Am 30.01.2018 um 08:55 schrieb Michal Hocko:
> On Tue 30-01-18 02:56:51, He, Roger wrote:
>> Hi Michal:
>>
>> We need a API to tell TTM module the system totally has how many swap
>> cache.  Then TTM module can use it to restrict how many the swap cache
>> it can use to prevent triggering OOM.  For Now we set the threshold of
>> swap size TTM used as 1/2 * total size and leave the rest for others
>> use.
> Why do you so much memory? Are you going to use TB of memory on large
> systems? What about memory hotplug when the memory is added/released?

For graphics and compute applications on GPUs it isn't unusual to use 
large amounts of system memory.

Our standard policy in TTM is to allow 50% of system memory to be pinned 
for use with GPUs (the hardware can't do page faults).

When that limit is exceeded (or the shrinker callbacks tell us to make 
room) we wait for any GPU work to finish and copy buffer content into a 
shmem file.

This copy into a shmem file can easily trigger the OOM killer if there 
isn't any swap space left and that is something we want to avoid.

So what we want to do is to apply this 50% rule to swap space as well 
and deny allocation of buffer objects when it is exceeded.

>> But get_nr_swap_pages is the only API we can accessed from other
>> module now.  It can't cover the case of the dynamic swap size
>> increment.  I mean: user can use "swapon" to enable new swap file or
>> swap disk dynamically or "swapoff" to disable swap space.
> Exactly. Your scaling configuration based on get_nr_swap_pages or the
> available memory simply sounds wrong.

Why? That is pretty much exactly what we are doing with buffer objects 
and system memory for years.

Regards,
Christian.

Michal Hocko Jan. 30, 2018, 10:18 a.m. UTC | #6

On Tue 30-01-18 10:00:07, Christian König wrote:
> Am 30.01.2018 um 08:55 schrieb Michal Hocko:
> > On Tue 30-01-18 02:56:51, He, Roger wrote:
> > > Hi Michal:
> > > 
> > > We need a API to tell TTM module the system totally has how many swap
> > > cache.  Then TTM module can use it to restrict how many the swap cache
> > > it can use to prevent triggering OOM.  For Now we set the threshold of
> > > swap size TTM used as 1/2 * total size and leave the rest for others
> > > use.
> > Why do you so much memory? Are you going to use TB of memory on large
> > systems? What about memory hotplug when the memory is added/released?
> 
> For graphics and compute applications on GPUs it isn't unusual to use large
> amounts of system memory.
> 
> Our standard policy in TTM is to allow 50% of system memory to be pinned for
> use with GPUs (the hardware can't do page faults).
> 
> When that limit is exceeded (or the shrinker callbacks tell us to make room)
> we wait for any GPU work to finish and copy buffer content into a shmem
> file.
> 
> This copy into a shmem file can easily trigger the OOM killer if there isn't
> any swap space left and that is something we want to avoid.
> 
> So what we want to do is to apply this 50% rule to swap space as well and
> deny allocation of buffer objects when it is exceeded.

How does that help when the rest of the system might eat swap?

> > > But get_nr_swap_pages is the only API we can accessed from other
> > > module now.  It can't cover the case of the dynamic swap size
> > > increment.  I mean: user can use "swapon" to enable new swap file or
> > > swap disk dynamically or "swapoff" to disable swap space.
> > Exactly. Your scaling configuration based on get_nr_swap_pages or the
> > available memory simply sounds wrong.
> 
> Why? That is pretty much exactly what we are doing with buffer objects and
> system memory for years.

Could you be more specific? What kind of buffer objects you have in
mind?

Christian König Jan. 30, 2018, 10:32 a.m. UTC | #7

Am 30.01.2018 um 11:18 schrieb Michal Hocko:
> On Tue 30-01-18 10:00:07, Christian König wrote:
>> Am 30.01.2018 um 08:55 schrieb Michal Hocko:
>>> On Tue 30-01-18 02:56:51, He, Roger wrote:
>>>> Hi Michal:
>>>>
>>>> We need a API to tell TTM module the system totally has how many swap
>>>> cache.  Then TTM module can use it to restrict how many the swap cache
>>>> it can use to prevent triggering OOM.  For Now we set the threshold of
>>>> swap size TTM used as 1/2 * total size and leave the rest for others
>>>> use.
>>> Why do you so much memory? Are you going to use TB of memory on large
>>> systems? What about memory hotplug when the memory is added/released?
>> For graphics and compute applications on GPUs it isn't unusual to use large
>> amounts of system memory.
>>
>> Our standard policy in TTM is to allow 50% of system memory to be pinned for
>> use with GPUs (the hardware can't do page faults).
>>
>> When that limit is exceeded (or the shrinker callbacks tell us to make room)
>> we wait for any GPU work to finish and copy buffer content into a shmem
>> file.
>>
>> This copy into a shmem file can easily trigger the OOM killer if there isn't
>> any swap space left and that is something we want to avoid.
>>
>> So what we want to do is to apply this 50% rule to swap space as well and
>> deny allocation of buffer objects when it is exceeded.
> How does that help when the rest of the system might eat swap?

Well it doesn't, but that is not the problem here.

When an application keeps calling malloc() it sooner or later is 
confronted with an OOM killer.

But when it keeps for example allocating OpenGL textures the expectation 
is that this sooner or later starts to fail because we run out of memory 
and not trigger the OOM killer.

So what we do is to allow the application to use all of video memory + a 
certain amount of system memory + swap space as last resort fallback 
(e.g. when you Alt+Tab from your full screen game back to your browser).

The problem we try to solve is that we haven't limited the use of swap 
space somehow.

>>>> But get_nr_swap_pages is the only API we can accessed from other
>>>> module now.  It can't cover the case of the dynamic swap size
>>>> increment.  I mean: user can use "swapon" to enable new swap file or
>>>> swap disk dynamically or "swapoff" to disable swap space.
>>> Exactly. Your scaling configuration based on get_nr_swap_pages or the
>>> available memory simply sounds wrong.
>> Why? That is pretty much exactly what we are doing with buffer objects and
>> system memory for years.
> Could you be more specific? What kind of buffer objects you have in
> mind?

Those are GEM buffer objects which user space uses for things like 
OpenGL textures, OpenCL matrix, Vulkan surfaces, video codec surfaces 
etc etc...

Regards,
Christian.

Michal Hocko Jan. 30, 2018, 12:28 p.m. UTC | #8

On Tue 30-01-18 11:32:49, Christian König wrote:
> Am 30.01.2018 um 11:18 schrieb Michal Hocko:
> > On Tue 30-01-18 10:00:07, Christian König wrote:
> > > Am 30.01.2018 um 08:55 schrieb Michal Hocko:
> > > > On Tue 30-01-18 02:56:51, He, Roger wrote:
> > > > > Hi Michal:
> > > > > 
> > > > > We need a API to tell TTM module the system totally has how many swap
> > > > > cache.  Then TTM module can use it to restrict how many the swap cache
> > > > > it can use to prevent triggering OOM.  For Now we set the threshold of
> > > > > swap size TTM used as 1/2 * total size and leave the rest for others
> > > > > use.
> > > > Why do you so much memory? Are you going to use TB of memory on large
> > > > systems? What about memory hotplug when the memory is added/released?
> > > For graphics and compute applications on GPUs it isn't unusual to use large
> > > amounts of system memory.
> > > 
> > > Our standard policy in TTM is to allow 50% of system memory to be pinned for
> > > use with GPUs (the hardware can't do page faults).
> > > 
> > > When that limit is exceeded (or the shrinker callbacks tell us to make room)
> > > we wait for any GPU work to finish and copy buffer content into a shmem
> > > file.
> > > 
> > > This copy into a shmem file can easily trigger the OOM killer if there isn't
> > > any swap space left and that is something we want to avoid.
> > > 
> > > So what we want to do is to apply this 50% rule to swap space as well and
> > > deny allocation of buffer objects when it is exceeded.
> > How does that help when the rest of the system might eat swap?
> 
> Well it doesn't, but that is not the problem here.
> 
> When an application keeps calling malloc() it sooner or later is confronted
> with an OOM killer.
> 
> But when it keeps for example allocating OpenGL textures the expectation is
> that this sooner or later starts to fail because we run out of memory and
> not trigger the OOM killer.

There is nothing like running out of memory and not triggering the OOM
killer. You can make a _particular_ allocation to bail out without the
oom killer. Just use __GFP_NORETRY. But that doesn't make much
difference when you have already depleted your memory and live with the
bare remainings. Any desperate soul trying to get its memory will simply
trigger the OOM.

> So what we do is to allow the application to use all of video memory + a
> certain amount of system memory + swap space as last resort fallback (e.g.
> when you Alt+Tab from your full screen game back to your browser).
> 
> The problem we try to solve is that we haven't limited the use of swap space
> somehow.

I do think you should completely ignore the size of the swap space. IMHO
you should forbid further allocations when your current buffer storage
cannot be reclaimed. So you need some form of feedback mechanism that
would tell you: "Your buffers have grown too much". If you cannot do
that then simply assume that you cannot swap at all rather than rely on
having some portion of it for yourself. There are many other users of
memory outside of your subsystem. Any scaling based on the 50% of resource
belonging to me is simply broken.

Christian König Jan. 30, 2018, 12:52 p.m. UTC | #9

Am 30.01.2018 um 13:28 schrieb Michal Hocko:
> I do think you should completely ignore the size of the swap space. IMHO
> you should forbid further allocations when your current buffer storage
> cannot be reclaimed. So you need some form of feedback mechanism that
> would tell you: "Your buffers have grown too much".

Yeah well, that is exactly what we are trying to do here.

> If you cannot do
> that then simply assume that you cannot swap at all rather than rely on
> having some portion of it for yourself. There are many other users of
> memory outside of your subsystem. Any scaling based on the 50% of resource
> belonging to me is simply broken.

Our intention is not reserve 50% of resources to TTM, but rather allow 
TTM to abort when more than 50% of all resources are used up.

Rogers initial implementation didn't looked like that, but that is just 
a minor mistake we can fix.

Regards,
Christian.

He, Hongbo Jan. 31, 2018, 5:52 a.m. UTC | #10

I do think you should completely ignore the size of the swap space. IMHO you should forbid further allocations when your current 	buffer storage cannot be reclaimed. So you need some form of feedback mechanism that would tell you: "Your buffers have 	grown too much". If you cannot do that then simply assume that you cannot swap at all rather than rely on having some portion 	of it for yourself. 

If we assume the swap cache size is zero always, that is overkill for GTT size actually user can get. And not make sense as well I think.

	There are many other users of memory outside of your subsystem. Any scaling based on the 50% of resource belonging to me is 	simply broken.

And that is only a threshold to avoid  overuse  rather than really reserved to TTM at the start. In addition, for most cases TTM only uses a little or not use swap disk at all. Only special test case use more or probably that is intentional.


Thanks
Roger(Hongbo.He)

-----Original Message-----
From: Michal Hocko [mailto:[e-mail]] 
Sent: Tuesday, January 30, 2018 8:29 PM
To: Koenig, Christian <[e-mail]>
Cc: He, Roger <[e-mail]>; [e-mail]; [e-mail]; [e-mail]
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

On Tue 30-01-18 11:32:49, Christian König wrote:
> Am 30.01.2018 um 11:18 schrieb Michal Hocko:
> > On Tue 30-01-18 10:00:07, Christian König wrote:
> > > Am 30.01.2018 um 08:55 schrieb Michal Hocko:
> > > > On Tue 30-01-18 02:56:51, He, Roger wrote:
> > > > > Hi Michal:
> > > > > 
> > > > > We need a API to tell TTM module the system totally has how 
> > > > > many swap cache.  Then TTM module can use it to restrict how 
> > > > > many the swap cache it can use to prevent triggering OOM.  For 
> > > > > Now we set the threshold of swap size TTM used as 1/2 * total 
> > > > > size and leave the rest for others use.
> > > > Why do you so much memory? Are you going to use TB of memory on 
> > > > large systems? What about memory hotplug when the memory is added/released?
> > > For graphics and compute applications on GPUs it isn't unusual to 
> > > use large amounts of system memory.
> > > 
> > > Our standard policy in TTM is to allow 50% of system memory to be 
> > > pinned for use with GPUs (the hardware can't do page faults).
> > > 
> > > When that limit is exceeded (or the shrinker callbacks tell us to 
> > > make room) we wait for any GPU work to finish and copy buffer 
> > > content into a shmem file.
> > > 
> > > This copy into a shmem file can easily trigger the OOM killer if 
> > > there isn't any swap space left and that is something we want to avoid.
> > > 
> > > So what we want to do is to apply this 50% rule to swap space as 
> > > well and deny allocation of buffer objects when it is exceeded.
> > How does that help when the rest of the system might eat swap?
> 
> Well it doesn't, but that is not the problem here.
> 
> When an application keeps calling malloc() it sooner or later is 
> confronted with an OOM killer.
> 
> But when it keeps for example allocating OpenGL textures the 
> expectation is that this sooner or later starts to fail because we run 
> out of memory and not trigger the OOM killer.

There is nothing like running out of memory and not triggering the OOM killer. You can make a _particular_ allocation to bail out without the oom killer. Just use __GFP_NORETRY. But that doesn't make much difference when you have already depleted your memory and live with the bare remainings. Any desperate soul trying to get its memory will simply trigger the OOM.

> So what we do is to allow the application to use all of video memory + 
> a certain amount of system memory + swap space as last resort fallback (e.g.
> when you Alt+Tab from your full screen game back to your browser).
> 
> The problem we try to solve is that we haven't limited the use of swap 
> space somehow.

I do think you should completely ignore the size of the swap space. IMHO you should forbid further allocations when your current buffer storage cannot be reclaimed. So you need some form of feedback mechanism that would tell you: "Your buffers have grown too much". If you cannot do that then simply assume that you cannot swap at all rather than rely on having some portion of it for yourself. There are many other users of memory outside of your subsystem. Any scaling based on the 50% of resource belonging to me is simply broken.
--
Michal Hocko
SUSE Labs

Chunming Zhou Jan. 31, 2018, 7:15 a.m. UTC | #11

Hi Roger,

I think this patch isn't need at all. You can directly read 
total_swap_pages variable in TTM. See the comment:

/* protected with swap_lock. reading in vm_swap_full() doesn't need lock */
long total_swap_pages;

there are many places using it directly, you just couldn't change its 
value. Reading it doesn't need lock.


Regards,

David Zhou


On 2018年01月29日 16:29, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.
>
> Signed-off-by: Roger He <[e-mail]>
> ---
>   include/linux/swap.h |  6 ++++++
>   mm/swapfile.c        | 15 +++++++++++++++
>   2 files changed, 21 insertions(+)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index c2b8128..708d66f 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>   struct backing_dev_info;
>   extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
>   extern void exit_swap_address_space(unsigned int type);
> +extern long get_total_swap_pages(void);
>   
>   #else /* CONFIG_SWAP */
>   
> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)
>   {
>   }
>   
> +long get_total_swap_pages(void)
> +{
> +	return 0;
> +}
> +
>   #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
>   #define swapcache_prepare(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
>   
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02..a0062eb 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>   
>   atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>   
> +/*
> + * expose this value for others use
> + */
> +long get_total_swap_pages(void)
> +{
> +	long ret;
> +
> +	spin_lock(&swap_lock);
> +	ret = total_swap_pages;
> +	spin_unlock(&swap_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
> +
>   static inline unsigned char swap_count(unsigned char ent)
>   {
>   	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

He, Hongbo Jan. 31, 2018, 8:08 a.m. UTC | #12

I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.

Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:
"WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".

Thanks
Roger(Hongbo.He)
-----Original Message-----
From: dri-devel [mailto:[e-mail]] On Behalf Of Chunming Zhou

Sent: Wednesday, January 31, 2018 3:15 PM
To: He, Roger <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]; Koenig, Christian <[e-mail]>
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Hi Roger,

I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:

/* protected with swap_lock. reading in vm_swap_full() doesn't need lock */ long total_swap_pages;

there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.


Regards,

David Zhou


On 2018年01月29日 16:29, Roger He wrote:
> ttm module needs it to determine its internal parameter setting.

>

> Signed-off-by: Roger He <[e-mail]>

> ---

>   include/linux/swap.h |  6 ++++++

>   mm/swapfile.c        | 15 +++++++++++++++

>   2 files changed, 21 insertions(+)

>

> diff --git a/include/linux/swap.h b/include/linux/swap.h

> index c2b8128..708d66f 100644

> --- a/include/linux/swap.h

> +++ b/include/linux/swap.h

> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);

>   struct backing_dev_info;

>   extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);

>   extern void exit_swap_address_space(unsigned int type);

> +extern long get_total_swap_pages(void);

>   

>   #else /* CONFIG_SWAP */

>   

> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)

>   {

>   }

>   

> +long get_total_swap_pages(void)

> +{

> +	return 0;

> +}

> +

>   #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})

>   #define swapcache_prepare(e) ({(is_migration_entry(e) || is_device_private_entry(e));})

>   

> diff --git a/mm/swapfile.c b/mm/swapfile.c

> index 3074b02..a0062eb 100644

> --- a/mm/swapfile.c

> +++ b/mm/swapfile.c

> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>   

>   atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>   

> +/*

> + * expose this value for others use

> + */

> +long get_total_swap_pages(void)

> +{

> +	long ret;

> +

> +	spin_lock(&swap_lock);

> +	ret = total_swap_pages;

> +	spin_unlock(&swap_lock);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

> +

>   static inline unsigned char swap_count(unsigned char ent)

>   {

>   	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */


_______________________________________________
dri-devel mailing list
[e-mail]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Christian König Jan. 31, 2018, 8:12 a.m. UTC | #13

Yeah, indeed. But what we could do is to rely on a fixed limit like the 
Intel driver does and I suggested before.

E.g. don't copy anything into a shmemfile when there is only x MB of 
swap space left.

Roger can you test that approach once more with your fix for the OOM 
issues in the page fault handler?

Thanks,
Christian.

Am 31.01.2018 um 09:08 schrieb He, Roger:
> 	I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.
>
> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:
> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".
>
> Thanks
> Roger(Hongbo.He)
> -----Original Message-----
> From: dri-devel [mailto:[e-mail]] On Behalf Of Chunming Zhou
> Sent: Wednesday, January 31, 2018 3:15 PM
> To: He, Roger <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; Koenig, Christian <[e-mail]>
> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages
>
> Hi Roger,
>
> I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:
>
> /* protected with swap_lock. reading in vm_swap_full() doesn't need lock */ long total_swap_pages;
>
> there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.
>
>
> Regards,
>
> David Zhou
>
>
> On 2018年01月29日 16:29, Roger He wrote:
>> ttm module needs it to determine its internal parameter setting.
>>
>> Signed-off-by: Roger He <[e-mail]>
>> ---
>>    include/linux/swap.h |  6 ++++++
>>    mm/swapfile.c        | 15 +++++++++++++++
>>    2 files changed, 21 insertions(+)
>>
>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>> index c2b8128..708d66f 100644
>> --- a/include/linux/swap.h
>> +++ b/include/linux/swap.h
>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>>    struct backing_dev_info;
>>    extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
>>    extern void exit_swap_address_space(unsigned int type);
>> +extern long get_total_swap_pages(void);
>>    
>>    #else /* CONFIG_SWAP */
>>    
>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)
>>    {
>>    }
>>    
>> +long get_total_swap_pages(void)
>> +{
>> +	return 0;
>> +}
>> +
>>    #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
>>    #define swapcache_prepare(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
>>    
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 3074b02..a0062eb 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>>    
>>    atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>>    
>> +/*
>> + * expose this value for others use
>> + */
>> +long get_total_swap_pages(void)
>> +{
>> +	long ret;
>> +
>> +	spin_lock(&swap_lock);
>> +	ret = total_swap_pages;
>> +	spin_unlock(&swap_lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
>> +
>>    static inline unsigned char swap_count(unsigned char ent)
>>    {
>>    	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Chunming Zhou Jan. 31, 2018, 8:52 a.m. UTC | #14

Discussed with Roger just now, we can try "void si_swapinfo(struct 
sysinfo *val)" function to get the total swap space.

Regards,

David Zhou


On 2018年01月31日 16:12, Christian König wrote:
> Yeah, indeed. But what we could do is to rely on a fixed limit like 
> the Intel driver does and I suggested before.
>
> E.g. don't copy anything into a shmemfile when there is only x MB of 
> swap space left.
>
> Roger can you test that approach once more with your fix for the OOM 
> issues in the page fault handler?
>
> Thanks,
> Christian.
>
> Am 31.01.2018 um 09:08 schrieb He, Roger:
>>     I think this patch isn't need at all. You can directly read 
>> total_swap_pages variable in TTM.
>>
>> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct 
>> using will result in:
>> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".
>>
>> Thanks
>> Roger(Hongbo.He)
>> -----Original Message-----
>> From: dri-devel [mailto:[e-mail]] On 
>> Behalf Of Chunming Zhou
>> Sent: Wednesday, January 31, 2018 3:15 PM
>> To: He, Roger <[e-mail]>; [e-mail]
>> Cc: [e-mail]; [e-mail]; Koenig, 
>> Christian <[e-mail]>
>> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 
>> expose total_swap_pages
>>
>> Hi Roger,
>>
>> I think this patch isn't need at all. You can directly read 
>> total_swap_pages variable in TTM. See the comment:
>>
>> /* protected with swap_lock. reading in vm_swap_full() doesn't need 
>> lock */ long total_swap_pages;
>>
>> there are many places using it directly, you just couldn't change its 
>> value. Reading it doesn't need lock.
>>
>>
>> Regards,
>>
>> David Zhou
>>
>>
>> On 2018年01月29日 16:29, Roger He wrote:
>>> ttm module needs it to determine its internal parameter setting.
>>>
>>> Signed-off-by: Roger He <[e-mail]>
>>> ---
>>>    include/linux/swap.h |  6 ++++++
>>>    mm/swapfile.c        | 15 +++++++++++++++
>>>    2 files changed, 21 insertions(+)
>>>
>>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>>> index c2b8128..708d66f 100644
>>> --- a/include/linux/swap.h
>>> +++ b/include/linux/swap.h
>>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>>>    struct backing_dev_info;
>>>    extern int init_swap_address_space(unsigned int type, unsigned 
>>> long nr_pages);
>>>    extern void exit_swap_address_space(unsigned int type);
>>> +extern long get_total_swap_pages(void);
>>>       #else /* CONFIG_SWAP */
>>>    @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)
>>>    {
>>>    }
>>>    +long get_total_swap_pages(void)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>    #define free_swap_and_cache(e) ({(is_migration_entry(e) || 
>>> is_device_private_entry(e));})
>>>    #define swapcache_prepare(e) ({(is_migration_entry(e) || 
>>> is_device_private_entry(e));})
>>>    diff --git a/mm/swapfile.c b/mm/swapfile.c
>>> index 3074b02..a0062eb 100644
>>> --- a/mm/swapfile.c
>>> +++ b/mm/swapfile.c
>>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>>>       atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>>>    +/*
>>> + * expose this value for others use
>>> + */
>>> +long get_total_swap_pages(void)
>>> +{
>>> +    long ret;
>>> +
>>> +    spin_lock(&swap_lock);
>>> +    ret = total_swap_pages;
>>> +    spin_unlock(&swap_lock);
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
>>> +
>>>    static inline unsigned char swap_count(unsigned char ent)
>>>    {
>>>        return ent & ~SWAP_HAS_CACHE;    /* may include SWAP_HAS_CONT 
>>> flag */
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

He, Hongbo Feb. 1, 2018, 5:48 a.m. UTC | #15

But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.
	E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

Here I think we can do it further, let the limit value scaling with total system memory.
For example: total system memory * 1/2. 
If that it will match the platform configuration better. 

	Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

Sure. Use the limit as total ram*1/2 seems work very well. 
No OOM although swap disk reaches full at peak for piglit test.
I speculate this case happens but no OOM because:

a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 * total.
b. all subsequent swapped pages stay in system memory until no space there.
     Then the swapped pages in shmem be flushed into swap disk. And probably OS also need some swap space.
     For this case, it is easy to get full for swap disk.
c. but because now free swap size < 1/2 * total, so no swap out happen  after that. 
    And at least 1/4* system memory will left because below check in ttm_mem_global_reserve will ensure that.
	if (zone->used_mem > limit)
			goto out_unlock;
    
Thanks
Roger(Hongbo.He)
-----Original Message-----
From: Koenig, Christian 

Sent: Wednesday, January 31, 2018 4:13 PM
To: He, Roger <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.

E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

Thanks,
Christian.

Am 31.01.2018 um 09:08 schrieb He, Roger:
> 	I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.

>

> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:

> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".

>

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: dri-devel [mailto:[e-mail]] On 

> Behalf Of Chunming Zhou

> Sent: Wednesday, January 31, 2018 3:15 PM

> To: He, Roger <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]; Koenig, 

> Christian <[e-mail]>

> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> Hi Roger,

>

> I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:

>

> /* protected with swap_lock. reading in vm_swap_full() doesn't need 

> lock */ long total_swap_pages;

>

> there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.

>

>

> Regards,

>

> David Zhou

>

>

> On 2018年01月29日 16:29, Roger He wrote:

>> ttm module needs it to determine its internal parameter setting.

>>

>> Signed-off-by: Roger He <[e-mail]>

>> ---

>>    include/linux/swap.h |  6 ++++++

>>    mm/swapfile.c        | 15 +++++++++++++++

>>    2 files changed, 21 insertions(+)

>>

>> diff --git a/include/linux/swap.h b/include/linux/swap.h index 

>> c2b8128..708d66f 100644

>> --- a/include/linux/swap.h

>> +++ b/include/linux/swap.h

>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);

>>    struct backing_dev_info;

>>    extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);

>>    extern void exit_swap_address_space(unsigned int type);

>> +extern long get_total_swap_pages(void);

>>    

>>    #else /* CONFIG_SWAP */

>>    

>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)

>>    {

>>    }

>>    

>> +long get_total_swap_pages(void)

>> +{

>> +	return 0;

>> +}

>> +

>>    #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})

>>    #define swapcache_prepare(e) ({(is_migration_entry(e) || 

>> is_device_private_entry(e));})

>>    

>> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb 

>> 100644

>> --- a/mm/swapfile.c

>> +++ b/mm/swapfile.c

>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>>    

>>    atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>>    

>> +/*

>> + * expose this value for others use

>> + */

>> +long get_total_swap_pages(void)

>> +{

>> +	long ret;

>> +

>> +	spin_lock(&swap_lock);

>> +	ret = total_swap_pages;

>> +	spin_unlock(&swap_lock);

>> +

>> +	return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

>> +

>>    static inline unsigned char swap_count(unsigned char ent)

>>    {

>>    	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

> _______________________________________________

> dri-devel mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/dri-devel

He, Hongbo Feb. 1, 2018, 6:13 a.m. UTC | #16

Hi Michal:

How about only  
EXPORT_SYMBOL_GPL(total_swap_pages) ?

Thanks
Roger(Hongbo.He)

-----Original Message-----
From: He, Roger 
Sent: Wednesday, January 31, 2018 1:52 PM
To: 'Michal Hocko' <[e-mail]>; Koenig, Christian <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

	I do think you should completely ignore the size of the swap space. IMHO you should forbid further allocations when your current 	buffer storage cannot be reclaimed. So you need some form of feedback mechanism that would tell you: "Your buffers have 	grown too much". If you cannot do that then simply assume that you cannot swap at all rather than rely on having some portion 	of it for yourself. 

If we assume the swap cache size is zero always, that is overkill for GTT size actually user can get. And not make sense as well I think.

	There are many other users of memory outside of your subsystem. Any scaling based on the 50% of resource belonging to me is 	simply broken.

And that is only a threshold to avoid  overuse  rather than really reserved to TTM at the start. In addition, for most cases TTM only uses a little or not use swap disk at all. Only special test case use more or probably that is intentional.


Thanks
Roger(Hongbo.He)

-----Original Message-----
From: Michal Hocko [mailto:[e-mail]]
Sent: Tuesday, January 30, 2018 8:29 PM
To: Koenig, Christian <[e-mail]>
Cc: He, Roger <[e-mail]>; [e-mail]; [e-mail]; [e-mail]
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

On Tue 30-01-18 11:32:49, Christian König wrote:
> Am 30.01.2018 um 11:18 schrieb Michal Hocko:
> > On Tue 30-01-18 10:00:07, Christian König wrote:
> > > Am 30.01.2018 um 08:55 schrieb Michal Hocko:
> > > > On Tue 30-01-18 02:56:51, He, Roger wrote:
> > > > > Hi Michal:
> > > > > 
> > > > > We need a API to tell TTM module the system totally has how 
> > > > > many swap cache.  Then TTM module can use it to restrict how 
> > > > > many the swap cache it can use to prevent triggering OOM.  For 
> > > > > Now we set the threshold of swap size TTM used as 1/2 * total 
> > > > > size and leave the rest for others use.
> > > > Why do you so much memory? Are you going to use TB of memory on 
> > > > large systems? What about memory hotplug when the memory is added/released?
> > > For graphics and compute applications on GPUs it isn't unusual to 
> > > use large amounts of system memory.
> > > 
> > > Our standard policy in TTM is to allow 50% of system memory to be 
> > > pinned for use with GPUs (the hardware can't do page faults).
> > > 
> > > When that limit is exceeded (or the shrinker callbacks tell us to 
> > > make room) we wait for any GPU work to finish and copy buffer 
> > > content into a shmem file.
> > > 
> > > This copy into a shmem file can easily trigger the OOM killer if 
> > > there isn't any swap space left and that is something we want to avoid.
> > > 
> > > So what we want to do is to apply this 50% rule to swap space as 
> > > well and deny allocation of buffer objects when it is exceeded.
> > How does that help when the rest of the system might eat swap?
> 
> Well it doesn't, but that is not the problem here.
> 
> When an application keeps calling malloc() it sooner or later is 
> confronted with an OOM killer.
> 
> But when it keeps for example allocating OpenGL textures the 
> expectation is that this sooner or later starts to fail because we run 
> out of memory and not trigger the OOM killer.

There is nothing like running out of memory and not triggering the OOM killer. You can make a _particular_ allocation to bail out without the oom killer. Just use __GFP_NORETRY. But that doesn't make much difference when you have already depleted your memory and live with the bare remainings. Any desperate soul trying to get its memory will simply trigger the OOM.

> So what we do is to allow the application to use all of video memory + 
> a certain amount of system memory + swap space as last resort fallback (e.g.
> when you Alt+Tab from your full screen game back to your browser).
> 
> The problem we try to solve is that we haven't limited the use of swap 
> space somehow.

I do think you should completely ignore the size of the swap space. IMHO you should forbid further allocations when your current buffer storage cannot be reclaimed. So you need some form of feedback mechanism that would tell you: "Your buffers have grown too much". If you cannot do that then simply assume that you cannot swap at all rather than rely on having some portion of it for yourself. There are many other users of memory outside of your subsystem. Any scaling based on the 50% of resource belonging to me is simply broken.
--
Michal Hocko
SUSE Labs

He, Hongbo Feb. 1, 2018, 8:03 a.m. UTC | #17

Just now, I tried with fixed limit.  But not work always.
For example: set the limit as 4GB on my platform with 8GB system memory, it can pass.
But when run with platform with 16GB system memory, it failed since OOM.

And I guess it also depends on app's behavior.
I mean some apps  make OS to use more swap space as well.

Thanks
Roger(Hongbo.He)
-----Original Message-----
From: dri-devel [mailto:[e-mail]] On Behalf Of He, Roger

Sent: Thursday, February 01, 2018 1:48 PM
To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

	But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.
	E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

Here I think we can do it further, let the limit value scaling with total system memory.
For example: total system memory * 1/2. 
If that it will match the platform configuration better. 

	Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

Sure. Use the limit as total ram*1/2 seems work very well. 
No OOM although swap disk reaches full at peak for piglit test.
I speculate this case happens but no OOM because:

a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 * total.
b. all subsequent swapped pages stay in system memory until no space there.
     Then the swapped pages in shmem be flushed into swap disk. And probably OS also need some swap space.
     For this case, it is easy to get full for swap disk.
c. but because now free swap size < 1/2 * total, so no swap out happen  after that. 
    And at least 1/4* system memory will left because below check in ttm_mem_global_reserve will ensure that.
	if (zone->used_mem > limit)
			goto out_unlock;
    
Thanks
Roger(Hongbo.He)
-----Original Message-----
From: Koenig, Christian

Sent: Wednesday, January 31, 2018 4:13 PM
To: He, Roger <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.

E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

Thanks,
Christian.

Am 31.01.2018 um 09:08 schrieb He, Roger:
> 	I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.

>

> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:

> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".

>

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: dri-devel [mailto:[e-mail]] On 

> Behalf Of Chunming Zhou

> Sent: Wednesday, January 31, 2018 3:15 PM

> To: He, Roger <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]; Koenig, 

> Christian <[e-mail]>

> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> Hi Roger,

>

> I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:

>

> /* protected with swap_lock. reading in vm_swap_full() doesn't need 

> lock */ long total_swap_pages;

>

> there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.

>

>

> Regards,

>

> David Zhou

>

>

> On 2018年01月29日 16:29, Roger He wrote:

>> ttm module needs it to determine its internal parameter setting.

>>

>> Signed-off-by: Roger He <[e-mail]>

>> ---

>>    include/linux/swap.h |  6 ++++++

>>    mm/swapfile.c        | 15 +++++++++++++++

>>    2 files changed, 21 insertions(+)

>>

>> diff --git a/include/linux/swap.h b/include/linux/swap.h index 

>> c2b8128..708d66f 100644

>> --- a/include/linux/swap.h

>> +++ b/include/linux/swap.h

>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);

>>    struct backing_dev_info;

>>    extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);

>>    extern void exit_swap_address_space(unsigned int type);

>> +extern long get_total_swap_pages(void);

>>    

>>    #else /* CONFIG_SWAP */

>>    

>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)

>>    {

>>    }

>>    

>> +long get_total_swap_pages(void)

>> +{

>> +	return 0;

>> +}

>> +

>>    #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})

>>    #define swapcache_prepare(e) ({(is_migration_entry(e) ||

>> is_device_private_entry(e));})

>>    

>> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb

>> 100644

>> --- a/mm/swapfile.c

>> +++ b/mm/swapfile.c

>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>>    

>>    atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>>    

>> +/*

>> + * expose this value for others use

>> + */

>> +long get_total_swap_pages(void)

>> +{

>> +	long ret;

>> +

>> +	spin_lock(&swap_lock);

>> +	ret = total_swap_pages;

>> +	spin_unlock(&swap_lock);

>> +

>> +	return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

>> +

>>    static inline unsigned char swap_count(unsigned char ent)

>>    {

>>    	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

> _______________________________________________

> dri-devel mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
[e-mail]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Michal Hocko Feb. 1, 2018, 8:15 a.m. UTC | #18

On Thu 01-02-18 06:13:20, He, Roger wrote:
> Hi Michal:
> 
> How about only  
> EXPORT_SYMBOL_GPL(total_swap_pages) ?

I've already expressed that messing up with the amount of swap pages is
a wrong approach. You should scale your additional buffers according the
the current memory pressure. There are other users of memory on the
system other than your subsystem.

He, Hongbo Feb. 2, 2018, 6:57 a.m. UTC | #19

Use the limit as total ram*1/2 seems work very well. 
	No OOM although swap disk reaches full at peak for piglit test.

But for this approach, David noticed that has an obvious defect. 
For example,  if the platform has 32G system memory, 8G swap disk.
1/2 * ram = 16G which is bigger than swap disk, so no swap for TTM is allowed at all.
For now we work out an improved version based on get_nr_swap_pages().
Going to send out later.

Thanks
Roger(Hongbo.He)
-----Original Message-----
From: He, Roger 

Sent: Thursday, February 01, 2018 4:03 PM
To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]; 'He, Roger' <[e-mail]>
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Just now, I tried with fixed limit.  But not work always.
For example: set the limit as 4GB on my platform with 8GB system memory, it can pass.
But when run with platform with 16GB system memory, it failed since OOM.

And I guess it also depends on app's behavior.
I mean some apps  make OS to use more swap space as well.

Thanks
Roger(Hongbo.He)
-----Original Message-----
From: dri-devel [mailto:[e-mail]] On Behalf Of He, Roger

Sent: Thursday, February 01, 2018 1:48 PM
To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

	But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.
	E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

Here I think we can do it further, let the limit value scaling with total system memory.
For example: total system memory * 1/2. 
If that it will match the platform configuration better. 

	Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

Sure. Use the limit as total ram*1/2 seems work very well. 
No OOM although swap disk reaches full at peak for piglit test.
I speculate this case happens but no OOM because:

a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 * total.
b. all subsequent swapped pages stay in system memory until no space there.
     Then the swapped pages in shmem be flushed into swap disk. And probably OS also need some swap space.
     For this case, it is easy to get full for swap disk.
c. but because now free swap size < 1/2 * total, so no swap out happen  after that. 
    And at least 1/4* system memory will left because below check in ttm_mem_global_reserve will ensure that.
	if (zone->used_mem > limit)
			goto out_unlock;
    
Thanks
Roger(Hongbo.He)
-----Original Message-----
From: Koenig, Christian

Sent: Wednesday, January 31, 2018 4:13 PM
To: He, Roger <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.

E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

Thanks,
Christian.

Am 31.01.2018 um 09:08 schrieb He, Roger:
> 	I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.

>

> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:

> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".

>

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: dri-devel [mailto:[e-mail]] On 

> Behalf Of Chunming Zhou

> Sent: Wednesday, January 31, 2018 3:15 PM

> To: He, Roger <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]; Koenig, 

> Christian <[e-mail]>

> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> Hi Roger,

>

> I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:

>

> /* protected with swap_lock. reading in vm_swap_full() doesn't need 

> lock */ long total_swap_pages;

>

> there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.

>

>

> Regards,

>

> David Zhou

>

>

> On 2018年01月29日 16:29, Roger He wrote:

>> ttm module needs it to determine its internal parameter setting.

>>

>> Signed-off-by: Roger He <[e-mail]>

>> ---

>>    include/linux/swap.h |  6 ++++++

>>    mm/swapfile.c        | 15 +++++++++++++++

>>    2 files changed, 21 insertions(+)

>>

>> diff --git a/include/linux/swap.h b/include/linux/swap.h index 

>> c2b8128..708d66f 100644

>> --- a/include/linux/swap.h

>> +++ b/include/linux/swap.h

>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);

>>    struct backing_dev_info;

>>    extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);

>>    extern void exit_swap_address_space(unsigned int type);

>> +extern long get_total_swap_pages(void);

>>    

>>    #else /* CONFIG_SWAP */

>>    

>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)

>>    {

>>    }

>>    

>> +long get_total_swap_pages(void)

>> +{

>> +	return 0;

>> +}

>> +

>>    #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})

>>    #define swapcache_prepare(e) ({(is_migration_entry(e) ||

>> is_device_private_entry(e));})

>>    

>> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb

>> 100644

>> --- a/mm/swapfile.c

>> +++ b/mm/swapfile.c

>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>>    

>>    atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>>    

>> +/*

>> + * expose this value for others use

>> + */

>> +long get_total_swap_pages(void)

>> +{

>> +	long ret;

>> +

>> +	spin_lock(&swap_lock);

>> +	ret = total_swap_pages;

>> +	spin_unlock(&swap_lock);

>> +

>> +	return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

>> +

>>    static inline unsigned char swap_count(unsigned char ent)

>>    {

>>    	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

> _______________________________________________

> dri-devel mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
[e-mail]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Christian König Feb. 2, 2018, 7:46 a.m. UTC | #20

Can you try to use a fixed limit like I suggested once more?

E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Regards,
Christian.

Am 02.02.2018 um 07:57 schrieb He, Roger:
> 	Use the limit as total ram*1/2 seems work very well.
> 	No OOM although swap disk reaches full at peak for piglit test.
>
> But for this approach, David noticed that has an obvious defect.
> For example,  if the platform has 32G system memory, 8G swap disk.
> 1/2 * ram = 16G which is bigger than swap disk, so no swap for TTM is allowed at all.
> For now we work out an improved version based on get_nr_swap_pages().
> Going to send out later.
>
> Thanks
> Roger(Hongbo.He)
> -----Original Message-----
> From: He, Roger
> Sent: Thursday, February 01, 2018 4:03 PM
> To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; 'He, Roger' <[e-mail]>
> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages
>
> Just now, I tried with fixed limit.  But not work always.
> For example: set the limit as 4GB on my platform with 8GB system memory, it can pass.
> But when run with platform with 16GB system memory, it failed since OOM.
>
> And I guess it also depends on app's behavior.
> I mean some apps  make OS to use more swap space as well.
>
> Thanks
> Roger(Hongbo.He)
> -----Original Message-----
> From: dri-devel [mailto:[e-mail]] On Behalf Of He, Roger
> Sent: Thursday, February 01, 2018 1:48 PM
> To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]
> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages
>
> 	But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.
> 	E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.
>
> Here I think we can do it further, let the limit value scaling with total system memory.
> For example: total system memory * 1/2.
> If that it will match the platform configuration better.
>
> 	Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?
>
> Sure. Use the limit as total ram*1/2 seems work very well.
> No OOM although swap disk reaches full at peak for piglit test.
> I speculate this case happens but no OOM because:
>
> a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 * total.
> b. all subsequent swapped pages stay in system memory until no space there.
>       Then the swapped pages in shmem be flushed into swap disk. And probably OS also need some swap space.
>       For this case, it is easy to get full for swap disk.
> c. but because now free swap size < 1/2 * total, so no swap out happen  after that.
>      And at least 1/4* system memory will left because below check in ttm_mem_global_reserve will ensure that.
> 	if (zone->used_mem > limit)
> 			goto out_unlock;
>      
> Thanks
> Roger(Hongbo.He)
> -----Original Message-----
> From: Koenig, Christian
> Sent: Wednesday, January 31, 2018 4:13 PM
> To: He, Roger <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]
> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages
>
> Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.
>
> E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.
>
> Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?
>
> Thanks,
> Christian.
>
> Am 31.01.2018 um 09:08 schrieb He, Roger:
>> 	I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.
>>
>> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:
>> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".
>>
>> Thanks
>> Roger(Hongbo.He)
>> -----Original Message-----
>> From: dri-devel [mailto:[e-mail]] On
>> Behalf Of Chunming Zhou
>> Sent: Wednesday, January 31, 2018 3:15 PM
>> To: He, Roger <[e-mail]>; [e-mail]
>> Cc: [e-mail]; [e-mail]; Koenig,
>> Christian <[e-mail]>
>> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to
>> expose total_swap_pages
>>
>> Hi Roger,
>>
>> I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:
>>
>> /* protected with swap_lock. reading in vm_swap_full() doesn't need
>> lock */ long total_swap_pages;
>>
>> there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.
>>
>>
>> Regards,
>>
>> David Zhou
>>
>>
>> On 2018年01月29日 16:29, Roger He wrote:
>>> ttm module needs it to determine its internal parameter setting.
>>>
>>> Signed-off-by: Roger He <[e-mail]>
>>> ---
>>>     include/linux/swap.h |  6 ++++++
>>>     mm/swapfile.c        | 15 +++++++++++++++
>>>     2 files changed, 21 insertions(+)
>>>
>>> diff --git a/include/linux/swap.h b/include/linux/swap.h index
>>> c2b8128..708d66f 100644
>>> --- a/include/linux/swap.h
>>> +++ b/include/linux/swap.h
>>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);
>>>     struct backing_dev_info;
>>>     extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
>>>     extern void exit_swap_address_space(unsigned int type);
>>> +extern long get_total_swap_pages(void);
>>>     
>>>     #else /* CONFIG_SWAP */
>>>     
>>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)
>>>     {
>>>     }
>>>     
>>> +long get_total_swap_pages(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>>     #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
>>>     #define swapcache_prepare(e) ({(is_migration_entry(e) ||
>>> is_device_private_entry(e));})
>>>     
>>> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb
>>> 100644
>>> --- a/mm/swapfile.c
>>> +++ b/mm/swapfile.c
>>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>>>     
>>>     atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>>>     
>>> +/*
>>> + * expose this value for others use
>>> + */
>>> +long get_total_swap_pages(void)
>>> +{
>>> +	long ret;
>>> +
>>> +	spin_lock(&swap_lock);
>>> +	ret = total_swap_pages;
>>> +	spin_unlock(&swap_lock);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);
>>> +
>>>     static inline unsigned char swap_count(unsigned char ent)
>>>     {
>>>     	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

He, Hongbo Feb. 2, 2018, 7:54 a.m. UTC | #21

Can you try to use a fixed limit like I suggested once more?
	E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Maybe you missed previous mail. I explain again here.
Set the value as 256MB not work on my platform.  My machine has 8GB system memory, and 8GB swap disk.
On my machine, set it as 4G can work.
But 4G also not work on test machine with 16GB system memory & 8GB swap disk.


Thanks
Roger(Hongbo.He)

-----Original Message-----
From: Koenig, Christian 

Sent: Friday, February 02, 2018 3:46 PM
To: He, Roger <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Can you try to use a fixed limit like I suggested once more?

E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Regards,
Christian.

Am 02.02.2018 um 07:57 schrieb He, Roger:
> 	Use the limit as total ram*1/2 seems work very well.

> 	No OOM although swap disk reaches full at peak for piglit test.

>

> But for this approach, David noticed that has an obvious defect.

> For example,  if the platform has 32G system memory, 8G swap disk.

> 1/2 * ram = 16G which is bigger than swap disk, so no swap for TTM is allowed at all.

> For now we work out an improved version based on get_nr_swap_pages().

> Going to send out later.

>

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: He, Roger

> Sent: Thursday, February 01, 2018 4:03 PM

> To: Koenig, Christian <[e-mail]>; Zhou, 

> David(ChunMing) <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]; 'He, Roger' 

> <[e-mail]>

> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> Just now, I tried with fixed limit.  But not work always.

> For example: set the limit as 4GB on my platform with 8GB system memory, it can pass.

> But when run with platform with 16GB system memory, it failed since OOM.

>

> And I guess it also depends on app's behavior.

> I mean some apps  make OS to use more swap space as well.

>

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: dri-devel [mailto:[e-mail]] On 

> Behalf Of He, Roger

> Sent: Thursday, February 01, 2018 1:48 PM

> To: Koenig, Christian <[e-mail]>; Zhou, 

> David(ChunMing) <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]

> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> 	But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.

> 	E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

>

> Here I think we can do it further, let the limit value scaling with total system memory.

> For example: total system memory * 1/2.

> If that it will match the platform configuration better.

>

> 	Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

>

> Sure. Use the limit as total ram*1/2 seems work very well.

> No OOM although swap disk reaches full at peak for piglit test.

> I speculate this case happens but no OOM because:

>

> a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 * total.

> b. all subsequent swapped pages stay in system memory until no space there.

>       Then the swapped pages in shmem be flushed into swap disk. And probably OS also need some swap space.

>       For this case, it is easy to get full for swap disk.

> c. but because now free swap size < 1/2 * total, so no swap out happen  after that.

>      And at least 1/4* system memory will left because below check in ttm_mem_global_reserve will ensure that.

> 	if (zone->used_mem > limit)

> 			goto out_unlock;

>      

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: Koenig, Christian

> Sent: Wednesday, January 31, 2018 4:13 PM

> To: He, Roger <[e-mail]>; Zhou, David(ChunMing) 

> <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]

> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.

>

> E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

>

> Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

>

> Thanks,

> Christian.

>

> Am 31.01.2018 um 09:08 schrieb He, Roger:

>> 	I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.

>>

>> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:

>> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".

>>

>> Thanks

>> Roger(Hongbo.He)

>> -----Original Message-----

>> From: dri-devel [mailto:[e-mail]] On 

>> Behalf Of Chunming Zhou

>> Sent: Wednesday, January 31, 2018 3:15 PM

>> To: He, Roger <[e-mail]>; [e-mail]

>> Cc: [e-mail]; [e-mail]; Koenig, 

>> Christian <[e-mail]>

>> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 

>> expose total_swap_pages

>>

>> Hi Roger,

>>

>> I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:

>>

>> /* protected with swap_lock. reading in vm_swap_full() doesn't need 

>> lock */ long total_swap_pages;

>>

>> there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.

>>

>>

>> Regards,

>>

>> David Zhou

>>

>>

>> On 2018年01月29日 16:29, Roger He wrote:

>>> ttm module needs it to determine its internal parameter setting.

>>>

>>> Signed-off-by: Roger He <[e-mail]>

>>> ---

>>>     include/linux/swap.h |  6 ++++++

>>>     mm/swapfile.c        | 15 +++++++++++++++

>>>     2 files changed, 21 insertions(+)

>>>

>>> diff --git a/include/linux/swap.h b/include/linux/swap.h index 

>>> c2b8128..708d66f 100644

>>> --- a/include/linux/swap.h

>>> +++ b/include/linux/swap.h

>>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);

>>>     struct backing_dev_info;

>>>     extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);

>>>     extern void exit_swap_address_space(unsigned int type);

>>> +extern long get_total_swap_pages(void);

>>>     

>>>     #else /* CONFIG_SWAP */

>>>     

>>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)

>>>     {

>>>     }

>>>     

>>> +long get_total_swap_pages(void)

>>> +{

>>> +	return 0;

>>> +}

>>> +

>>>     #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})

>>>     #define swapcache_prepare(e) ({(is_migration_entry(e) ||

>>> is_device_private_entry(e));})

>>>     

>>> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb

>>> 100644

>>> --- a/mm/swapfile.c

>>> +++ b/mm/swapfile.c

>>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>>>     

>>>     atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>>>     

>>> +/*

>>> + * expose this value for others use  */ long 

>>> +get_total_swap_pages(void) {

>>> +	long ret;

>>> +

>>> +	spin_lock(&swap_lock);

>>> +	ret = total_swap_pages;

>>> +	spin_unlock(&swap_lock);

>>> +

>>> +	return ret;

>>> +}

>>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

>>> +

>>>     static inline unsigned char swap_count(unsigned char ent)

>>>     {

>>>     	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

>> _______________________________________________

>> dri-devel mailing list

>> [e-mail]

>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

> _______________________________________________

> dri-devel mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/dri-devel

He, Hongbo Feb. 2, 2018, 8:59 a.m. UTC | #22

Probably it is necessary to summarize, and I have done below experiments:

A: use a fixed limit to stopping swapping out as below:
     int ttm_bo_swapout(struct ttm_bo_global *glob, struct ttm_operation_ctx *ctx)   {
	if get_nr_swap_pages() < 256MB  return no memory;
	....
     }
     Set the value as 256MB not work on my platform which has 8GB system memory & 8GB swap disk.
     Set it as 4GB can pass, but 4GB not work on test machine with 16GB system memory & 8GB swap disk.
     So set the limit as fixed value doesn't work always.

B. use the fixed limit as scaling with system memory, something as below:
     int ttm_bo_swapout(struct ttm_bo_global *glob, struct ttm_operation_ctx *ctx)   {
	if get_nr_swap_pages() < 1/2 * system memory size;
	....
     }
     So far, it can work on all test machine.
     But it has an obvious defect.
     For example,  if the platform has 32G system memory & 8G swap disk.
     1/2 * ram = 16G which is bigger than swap disk, so TTM swap for TTM is disallowed even when swap disk is empty at the start.
     So seems this approach is not reasonable. 

C. Then we work out new patches as in mailist today.


Thanks
Roger(Hongbo.He)
-----Original Message-----
From: dri-devel [mailto:[e-mail]] On Behalf Of He, Roger

Sent: Friday, February 02, 2018 3:54 PM
To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

	Can you try to use a fixed limit like I suggested once more?
	E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Maybe you missed previous mail. I explain again here.
Set the value as 256MB not work on my platform.  My machine has 8GB system memory, and 8GB swap disk.
On my machine, set it as 4G can work.
But 4G also not work on test machine with 16GB system memory & 8GB swap disk.


Thanks
Roger(Hongbo.He)

-----Original Message-----
From: Koenig, Christian

Sent: Friday, February 02, 2018 3:46 PM
To: He, Roger <[e-mail]>; Zhou, David(ChunMing) <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to expose total_swap_pages

Can you try to use a fixed limit like I suggested once more?

E.g. just stop swapping if get_nr_swap_pages() < 256MB.

Regards,
Christian.

Am 02.02.2018 um 07:57 schrieb He, Roger:
> 	Use the limit as total ram*1/2 seems work very well.

> 	No OOM although swap disk reaches full at peak for piglit test.

>

> But for this approach, David noticed that has an obvious defect.

> For example,  if the platform has 32G system memory, 8G swap disk.

> 1/2 * ram = 16G which is bigger than swap disk, so no swap for TTM is allowed at all.

> For now we work out an improved version based on get_nr_swap_pages().

> Going to send out later.

>

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: He, Roger

> Sent: Thursday, February 01, 2018 4:03 PM

> To: Koenig, Christian <[e-mail]>; Zhou,

> David(ChunMing) <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]; 'He, Roger' 

> <[e-mail]>

> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> Just now, I tried with fixed limit.  But not work always.

> For example: set the limit as 4GB on my platform with 8GB system memory, it can pass.

> But when run with platform with 16GB system memory, it failed since OOM.

>

> And I guess it also depends on app's behavior.

> I mean some apps  make OS to use more swap space as well.

>

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: dri-devel [mailto:[e-mail]] On 

> Behalf Of He, Roger

> Sent: Thursday, February 01, 2018 1:48 PM

> To: Koenig, Christian <[e-mail]>; Zhou,

> David(ChunMing) <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]

> Subject: RE: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> 	But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.

> 	E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

>

> Here I think we can do it further, let the limit value scaling with total system memory.

> For example: total system memory * 1/2.

> If that it will match the platform configuration better.

>

> 	Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

>

> Sure. Use the limit as total ram*1/2 seems work very well.

> No OOM although swap disk reaches full at peak for piglit test.

> I speculate this case happens but no OOM because:

>

> a. run a while, swap disk be used close to 1/2* total size and but not over 1/2 * total.

> b. all subsequent swapped pages stay in system memory until no space there.

>       Then the swapped pages in shmem be flushed into swap disk. And probably OS also need some swap space.

>       For this case, it is easy to get full for swap disk.

> c. but because now free swap size < 1/2 * total, so no swap out happen  after that.

>      And at least 1/4* system memory will left because below check in ttm_mem_global_reserve will ensure that.

> 	if (zone->used_mem > limit)

> 			goto out_unlock;

>      

> Thanks

> Roger(Hongbo.He)

> -----Original Message-----

> From: Koenig, Christian

> Sent: Wednesday, January 31, 2018 4:13 PM

> To: He, Roger <[e-mail]>; Zhou, David(ChunMing) 

> <[e-mail]>; [e-mail]

> Cc: [e-mail]; [e-mail]

> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 

> expose total_swap_pages

>

> Yeah, indeed. But what we could do is to rely on a fixed limit like the Intel driver does and I suggested before.

>

> E.g. don't copy anything into a shmemfile when there is only x MB of swap space left.

>

> Roger can you test that approach once more with your fix for the OOM issues in the page fault handler?

>

> Thanks,

> Christian.

>

> Am 31.01.2018 um 09:08 schrieb He, Roger:

>> 	I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM.

>>

>> Because the variable is not exported by EXPORT_SYMBOL_GPL. So direct using will result in:

>> "WARNING: "total_swap_pages" [drivers/gpu/drm/ttm/ttm.ko] undefined!".

>>

>> Thanks

>> Roger(Hongbo.He)

>> -----Original Message-----

>> From: dri-devel [mailto:[e-mail]] On 

>> Behalf Of Chunming Zhou

>> Sent: Wednesday, January 31, 2018 3:15 PM

>> To: He, Roger <[e-mail]>; [e-mail]

>> Cc: [e-mail]; [e-mail]; Koenig, 

>> Christian <[e-mail]>

>> Subject: Re: [PATCH] mm/swap: add function get_total_swap_pages to 

>> expose total_swap_pages

>>

>> Hi Roger,

>>

>> I think this patch isn't need at all. You can directly read total_swap_pages variable in TTM. See the comment:

>>

>> /* protected with swap_lock. reading in vm_swap_full() doesn't need 

>> lock */ long total_swap_pages;

>>

>> there are many places using it directly, you just couldn't change its value. Reading it doesn't need lock.

>>

>>

>> Regards,

>>

>> David Zhou

>>

>>

>> On 2018年01月29日 16:29, Roger He wrote:

>>> ttm module needs it to determine its internal parameter setting.

>>>

>>> Signed-off-by: Roger He <[e-mail]>

>>> ---

>>>     include/linux/swap.h |  6 ++++++

>>>     mm/swapfile.c        | 15 +++++++++++++++

>>>     2 files changed, 21 insertions(+)

>>>

>>> diff --git a/include/linux/swap.h b/include/linux/swap.h index 

>>> c2b8128..708d66f 100644

>>> --- a/include/linux/swap.h

>>> +++ b/include/linux/swap.h

>>> @@ -484,6 +484,7 @@ extern int try_to_free_swap(struct page *);

>>>     struct backing_dev_info;

>>>     extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);

>>>     extern void exit_swap_address_space(unsigned int type);

>>> +extern long get_total_swap_pages(void);

>>>     

>>>     #else /* CONFIG_SWAP */

>>>     

>>> @@ -516,6 +517,11 @@ static inline void show_swap_cache_info(void)

>>>     {

>>>     }

>>>     

>>> +long get_total_swap_pages(void)

>>> +{

>>> +	return 0;

>>> +}

>>> +

>>>     #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})

>>>     #define swapcache_prepare(e) ({(is_migration_entry(e) ||

>>> is_device_private_entry(e));})

>>>     

>>> diff --git a/mm/swapfile.c b/mm/swapfile.c index 3074b02..a0062eb

>>> 100644

>>> --- a/mm/swapfile.c

>>> +++ b/mm/swapfile.c

>>> @@ -98,6 +98,21 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);

>>>     

>>>     atomic_t nr_rotate_swap = ATOMIC_INIT(0);

>>>     

>>> +/*

>>> + * expose this value for others use  */ long

>>> +get_total_swap_pages(void) {

>>> +	long ret;

>>> +

>>> +	spin_lock(&swap_lock);

>>> +	ret = total_swap_pages;

>>> +	spin_unlock(&swap_lock);

>>> +

>>> +	return ret;

>>> +}

>>> +EXPORT_SYMBOL_GPL(get_total_swap_pages);

>>> +

>>>     static inline unsigned char swap_count(unsigned char ent)

>>>     {

>>>     	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */

>> _______________________________________________

>> dri-devel mailing list

>> [e-mail]

>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

> _______________________________________________

> dri-devel mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
[e-mail]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

diff --git a/include/linux/swap.h b/include/linux/swap.h
index c2b8128..708d66f 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -484,6 +484,7 @@  extern int try_to_free_swap(struct page *);
 struct backing_dev_info;
 extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
 extern void exit_swap_address_space(unsigned int type);
+extern long get_total_swap_pages(void);
 
 #else /* CONFIG_SWAP */
 
@@ -516,6 +517,11 @@  static inline void show_swap_cache_info(void)
 {
 }
 
+long get_total_swap_pages(void)
+{
+	return 0;
+}
+
 #define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
 #define swapcache_prepare(e) ({(is_migration_entry(e) || is_device_private_entry(e));})
 
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 3074b02..a0062eb 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -98,6 +98,21 @@  static atomic_t proc_poll_event = ATOMIC_INIT(0);
 
 atomic_t nr_rotate_swap = ATOMIC_INIT(0);
 
+/*
+ * expose this value for others use
+ */
+long get_total_swap_pages(void)
+{
+	long ret;
+
+	spin_lock(&swap_lock);
+	ret = total_swap_pages;
+	spin_unlock(&swap_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(get_total_swap_pages);
+
 static inline unsigned char swap_count(unsigned char ent)
 {
 	return ent & ~SWAP_HAS_CACHE;	/* may include SWAP_HAS_CONT flag */