[PATCH] drm/ttm: check if free mem space is under the lower limit

[Christian König]

Am 23.02.2018 um 03:18 schrieb He, Roger:
>
> -----Original Message-----
> From: Koenig, Christian
> Sent: Thursday, February 22, 2018 8:54 PM
> To: He, Roger <Hongbo.He at amd.com>; dri-devel at lists.freedesktop.org
> Subject: Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
>
> Am 22.02.2018 um 12:43 schrieb He, Roger:
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: Thursday, February 22, 2018 7:28 PM
>> To: He, Roger <Hongbo.He at amd.com>; dri-devel at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/ttm: check if free mem space is under the
>> lower limit
>>
>> Am 22.02.2018 um 11:10 schrieb Roger He:
>>> the free mem space and the lower limit both include two parts:
>>> system memory and swap space.
>>>
>>> For the OOM triggered by TTM, that is the case as below:
>>> first swap space is full of swapped out pages and soon system memory
>>> also is filled up with ttm pages. and then any memory allocation
>>> request will run into OOM.
>>>
>>> to cover two cases:
>>> a. if no swap disk at all or free swap space is under swap mem
>>>       limit but available system mem is bigger than sys mem limit,
>>>       allow TTM allocation;
>>>
>>> b. if the available system mem is less than sys mem limit but
>>>       free swap space is bigger than swap mem limit, allow TTM
>>>       allocation.
>>>
>>> v2: merge two memory limit(swap and system) into one
>>> v3: keep original behavior except ttm_opt_ctx->flags with
>>>        TTM_OPT_FLAG_FORCE_ALLOC
>>> v4: always set force_alloc as tx->flags & TTM_OPT_FLAG_FORCE_ALLOC
>>> v5: add an attribute for lower_mem_limit
>>>
>>> Signed-off-by: Roger He <Hongbo.He at amd.com>
>>> ---
>>>     drivers/gpu/drm/ttm/ttm_memory.c         | 94 ++++++++++++++++++++++++++++++++
>>>     drivers/gpu/drm/ttm/ttm_page_alloc.c     |  3 +
>>>     drivers/gpu/drm/ttm/ttm_page_alloc_dma.c |  3 +
>>>     include/drm/ttm/ttm_memory.h             |  5 ++
>>>     4 files changed, 105 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c
>>> b/drivers/gpu/drm/ttm/ttm_memory.c
>>> index aa0c381..d015e39 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_memory.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_memory.c
>>> @@ -36,6 +36,7 @@
>>>     #include <linux/mm.h>
>>>     #include <linux/module.h>
>>>     #include <linux/slab.h>
>>> +#include <linux/swap.h>
>>>     
>>>     #define TTM_MEMORY_ALLOC_RETRIES 4
>>>     
>>> @@ -166,6 +167,52 @@ static struct kobj_type ttm_mem_zone_kobj_type = {
>>>     	.default_attrs = ttm_mem_zone_attrs,
>>>     };
>>>     
>>> +static struct attribute ttm_mem_global_lower_mem_limit = {
>>> +	.name = "lower_mem_limit",
>>> +	.mode = S_IRUGO | S_IWUSR
>>> +};
>>> +
>>> +static ssize_t ttm_mem_global_show(struct kobject *kobj,
>>> +				 struct attribute *attr,
>>> +				 char *buffer)
>>> +{
>>> +	struct ttm_mem_global *glob =
>>> +		container_of(kobj, struct ttm_mem_global, kobj);
>>> +	uint64_t val = 0;
>>> +
>>> +	spin_lock(&glob->lock);
>>> +	val = glob->lower_mem_limit;
>>> +	spin_unlock(&glob->lock);
>>> +
>>> +	return snprintf(buffer, PAGE_SIZE, "%llu\n",
>>> +			(unsigned long long) val << 2);
>> 	What is that shift good for?
>>
>> Because the unit of lower_mem_limit is 4KB, so (val << 2) can get KB for consistent with other parameters as below(all are showed with KB bytes.):
> Ok that makes sense.
>
>> static struct attribute *ttm_mem_zone_attrs[] = {
>> 	&ttm_mem_sys,
>> 	&ttm_mem_emer,
>> 	&ttm_mem_max,
>> 	&ttm_mem_swap,
>> 	&ttm_mem_used,
>> 	NULL
>> };
>>    
>>> +}
>>> +
>>> +static ssize_t ttm_mem_global_store(struct kobject *kobj,
>>> +				  struct attribute *attr,
>>> +				  const char *buffer,
>>> +				  size_t size)
>>> +{
>>> +	int chars;
>>> +	uint64_t val64;
>>> +	unsigned long val;
>>> +	struct ttm_mem_global *glob =
>>> +		container_of(kobj, struct ttm_mem_global, kobj);
>>> +
>>> +	chars = sscanf(buffer, "%lu", &val);
>>> +	if (chars == 0)
>>> +		return size;
>>> +
>>> +	val64 = val;
>>> +	val64 >>= 2;
>> 	Dito?
>> Covert from KB to 4K page size here.
>>
>>> +
>>> +	spin_lock(&glob->lock);
>>> +	glob->lower_mem_limit = val64;
>>> +	spin_unlock(&glob->lock);
>>> +
>>> +	return size;
>>> +}
>>> +
>>>     static void ttm_mem_global_kobj_release(struct kobject *kobj)
>>>     {
>>>     	struct ttm_mem_global *glob =
>>> @@ -174,8 +221,20 @@ static void ttm_mem_global_kobj_release(struct kobject *kobj)
>>>     	kfree(glob);
>>>     }
>>>     
>>> +static struct attribute *ttm_mem_global_attrs[] = {
>>> +	&ttm_mem_global_lower_mem_limit,
>>> +	NULL
>>> +};
>>> +
>>> +static const struct sysfs_ops ttm_mem_global_ops = {
>>> +	.show = &ttm_mem_global_show,
>>> +	.store = &ttm_mem_global_store,
>>> +};
>>> +
>>>     static struct kobj_type ttm_mem_glob_kobj_type = {
>>>     	.release = &ttm_mem_global_kobj_release,
>>> +	.sysfs_ops = &ttm_mem_global_ops,
>>> +	.default_attrs = ttm_mem_global_attrs,
>>>     };
>>>     
>>>     static bool ttm_zones_above_swap_target(struct ttm_mem_global
>>> *glob, @@ -375,6 +434,11 @@ int ttm_mem_global_init(struct
>>> ttm_mem_global
>>> *glob)
>>>     
>>>     	si_meminfo(&si);
>>>     
>>> +	/* lower limit of swap space and 256MB is enough */
>>> +	glob->lower_mem_limit = 256 << 8;
>>> +	/* lower limit of ram and keep consistent with each zone->emer_mem */
>>> +	glob->lower_mem_limit += si.totalram >> 2;
>>> +
>> 	Mhm, I fear we need to set this to zero by default.
>>
>> Do you mean:  glob->lower_mem_limit = 0  ?
>> I don't get your point here. Then how amdgpu set it with above value to prevent OOM?
> 	We can't activate that by default because most use cases actually need the OOM behavior.
>
> 	So this should only be active for the customers especially requesting it.
>
> We have another option:  only enable this feature for AMDGPU.
>
> By default we set glob->lower_mem_limit as above rather than zero, but we add extra condition check as below in ttm_dma_populate/ttm_pool_populate:
>
> 	if ((ttm->page_flags & TTM_PAGE_FLAG_NO_RETRY) &&
> 	    ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
> 		return -ENOMEM;
>
> because only AMDGPU set TTM_PAGE_FLAG_NO_RETRY by default.
> Which options do you prefer?


Yeah, thought about that as well but I don't think we can do this.

Going a step further I actually think we should make the 
TTM_PAGE_FLAG_NO_RETRY depend on the limit and not set it by amdgpu any 
more.

But that can come later on.

Regards,
Christian.

>
> Thanks
> Roger(Hongbo.He)