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...@amd.com>; dri-devel@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...@amd.com>; dri-devel@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...@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)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to