Nouveau - Jun 2022 - [PATCH 03/13] mm: shmem: provide oom badness for shmem files

Am 09.06.22 um 11:18 schrieb Michal Hocko:
> On Tue 31-05-22 11:59:57, Christian K?nig wrote:
>> This gives the OOM killer an additional hint which processes are
>> referencing shmem files with potentially no other accounting for them.
>>
>> Signed-off-by: Christian K?nig <christian.koenig at amd.com>
>> ---
>>   mm/shmem.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 4b2fea33158e..a4ad92a16968 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -2179,6 +2179,11 @@ unsigned long shmem_get_unmapped_area(struct
file *file,
>>   	return inflated_addr;
>>   }
>>   
>> +static long shmem_oom_badness(struct file *file)
>> +{
>> +	return i_size_read(file_inode(file)) >> PAGE_SHIFT;
>> +}
> This doesn't really represent the in memory size of the file, does it?
Well the file could be partially or fully swapped out as anonymous 
memory or the address space only sparse populated, but even then just 
using the file size as OOM badness sounded like the most straightforward 
approach to me.

What could happen is that the file is also mmaped and we double account.
> Also the memcg oom handling could be considerably skewed if the file was
> shared between more memcgs.
Yes, and that's one of the reasons why I didn't touched the memcg by 
this and only affected the classic OOM killer.

Thanks for the comments,
Christian.

On Thu 09-06-22 14:16:56, Christian K?nig wrote:
> Am 09.06.22 um 11:18 schrieb Michal Hocko:
> > On Tue 31-05-22 11:59:57, Christian K?nig wrote:
> > > This gives the OOM killer an additional hint which processes are
> > > referencing shmem files with potentially no other accounting for
them.
> > > 
> > > Signed-off-by: Christian K?nig <christian.koenig at
amd.com>
> > > ---
> > >   mm/shmem.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index 4b2fea33158e..a4ad92a16968 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -2179,6 +2179,11 @@ unsigned long
shmem_get_unmapped_area(struct file *file,
> > >   	return inflated_addr;
> > >   }
> > > +static long shmem_oom_badness(struct file *file)
> > > +{
> > > +	return i_size_read(file_inode(file)) >> PAGE_SHIFT;
> > > +}
> > This doesn't really represent the in memory size of the file, does
it?
> 
> Well the file could be partially or fully swapped out as anonymous memory
or
> the address space only sparse populated, but even then just using the file
> size as OOM badness sounded like the most straightforward approach to me.
It covers hole as well, right?
> What could happen is that the file is also mmaped and we double account.
> 
> > Also the memcg oom handling could be considerably skewed if the file
was
> > shared between more memcgs.
> 
> Yes, and that's one of the reasons why I didn't touched the memcg
by this
> and only affected the classic OOM killer.
oom_badness is for all oom handlers, including memcg. Maybe I have
misread an earlier patch but I do not see anything specific to global
oom handling.

-- 
Michal Hocko
SUSE Labs

Am 09.06.22 um 14:57 schrieb Michal Hocko:
> On Thu 09-06-22 14:16:56, Christian K?nig wrote:
>> Am 09.06.22 um 11:18 schrieb Michal Hocko:
>>> On Tue 31-05-22 11:59:57, Christian K?nig wrote:
>>>> This gives the OOM killer an additional hint which processes
are
>>>> referencing shmem files with potentially no other accounting
for them.
>>>>
>>>> Signed-off-by: Christian K?nig <christian.koenig at
amd.com>
>>>> ---
>>>>    mm/shmem.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>> index 4b2fea33158e..a4ad92a16968 100644
>>>> --- a/mm/shmem.c
>>>> +++ b/mm/shmem.c
>>>> @@ -2179,6 +2179,11 @@ unsigned long
shmem_get_unmapped_area(struct file *file,
>>>>    	return inflated_addr;
>>>>    }
>>>> +static long shmem_oom_badness(struct file *file)
>>>> +{
>>>> +	return i_size_read(file_inode(file)) >> PAGE_SHIFT;
>>>> +}
>>> This doesn't really represent the in memory size of the file,
does it?
>> Well the file could be partially or fully swapped out as anonymous
memory or
>> the address space only sparse populated, but even then just using the
file
>> size as OOM badness sounded like the most straightforward approach to
me.
> It covers hole as well, right?
Yes, exactly.
>
>> What could happen is that the file is also mmaped and we double
account.
>>
>>> Also the memcg oom handling could be considerably skewed if the
file was
>>> shared between more memcgs.
>> Yes, and that's one of the reasons why I didn't touched the
memcg by this
>> and only affected the classic OOM killer.
> oom_badness is for all oom handlers, including memcg. Maybe I have
> misread an earlier patch but I do not see anything specific to global
> oom handling.
As far as I can see the oom_badness() function is only used in oom_kill.c and in
procfs to return the oom score. Did I missed something?

Regards,
Christian.