* [PATCH] drm/ttm: check if free mem space is under the lower limit
@ 2018-02-22 10:10 Roger He
2018-02-22 11:27 ` Christian König
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Roger He @ 2018-02-22 10:10 UTC (permalink / raw)
To: dri-devel; +Cc: Roger He, Christian.Koenig
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@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);
+}
+
+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;
+
+ 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;
+
ret = ttm_mem_init_kernel_zone(glob, &si);
if (unlikely(ret != 0))
goto out_no_zone;
@@ -469,6 +533,36 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
}
EXPORT_SYMBOL(ttm_mem_global_free);
+/*
+ * check if the available mem is under lower memory limit
+ *
+ * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
+ */
+bool
+ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
+ uint64_t num_pages,
+ struct ttm_operation_ctx *ctx)
+{
+ bool ret = false;
+ int64_t available;
+
+ if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
+ return false;
+
+ available = get_nr_swap_pages() + si_mem_available();
+ available -= num_pages;
+ if (available < glob->lower_mem_limit)
+ ret = true;
+
+ return ret;
+}
+EXPORT_SYMBOL(ttm_check_under_lowerlimit);
+
static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
struct ttm_mem_zone *single_zone,
uint64_t amount, bool reserve)
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 5edcd89..c9d8903 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
if (ttm->state != tt_unpopulated)
return 0;
+ if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
+ return -ENOMEM;
+
ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
ttm->caching_state);
if (unlikely(ret != 0)) {
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index b122f6e..37e03d9 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
if (ttm->state != tt_unpopulated)
return 0;
+ if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
+ return -ENOMEM;
+
INIT_LIST_HEAD(&ttm_dma->pages_list);
i = 0;
diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
index 8936285..737b5fe 100644
--- a/include/drm/ttm/ttm_memory.h
+++ b/include/drm/ttm/ttm_memory.h
@@ -49,6 +49,8 @@
* @work: The workqueue callback for the shrink queue.
* @lock: Lock to protect the @shrink - and the memory accounting members,
* that is, essentially the whole structure with some exceptions.
+ * @lower_mem_limit: include lower limit of swap space and lower limit of
+ * system memory.
* @zones: Array of pointers to accounting zones.
* @num_zones: Number of populated entries in the @zones array.
* @zone_kernel: Pointer to the kernel zone.
@@ -67,6 +69,7 @@ struct ttm_mem_global {
struct workqueue_struct *swap_queue;
struct work_struct work;
spinlock_t lock;
+ uint64_t lower_mem_limit;
struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
unsigned int num_zones;
struct ttm_mem_zone *zone_kernel;
@@ -90,4 +93,6 @@ extern void ttm_mem_global_free_page(struct ttm_mem_global *glob,
struct page *page, uint64_t size);
extern size_t ttm_round_pot(size_t size);
extern uint64_t ttm_get_kernel_zone_memory_size(struct ttm_mem_global *glob);
+extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
+ uint64_t num_pages, struct ttm_operation_ctx *ctx);
#endif
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 10:10 [PATCH] drm/ttm: check if free mem space is under the lower limit Roger He
@ 2018-02-22 11:27 ` Christian König
2018-02-22 11:43 ` He, Roger
2018-02-23 5:46 ` kbuild test robot
2018-02-23 8:49 ` kbuild test robot
2 siblings, 1 reply; 13+ messages in thread
From: Christian König @ 2018-02-22 11:27 UTC (permalink / raw)
To: Roger He, dri-devel
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@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?
> +}
> +
> +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?
> +
> + 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.
> ret = ttm_mem_init_kernel_zone(glob, &si);
> if (unlikely(ret != 0))
> goto out_no_zone;
> @@ -469,6 +533,36 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
> }
> EXPORT_SYMBOL(ttm_mem_global_free);
>
> +/*
> + * check if the available mem is under lower memory limit
> + *
> + * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
> + */
> +bool
> +ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
> + uint64_t num_pages,
> + struct ttm_operation_ctx *ctx)
> +{
> + bool ret = false;
> + int64_t available;
> +
> + if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
> + return false;
> +
> + available = get_nr_swap_pages() + si_mem_available();
> + available -= num_pages;
> + if (available < glob->lower_mem_limit)
> + ret = true;
> +
> + return ret;
Don't use a local variable, just use "return true" / "return false;".
Regards,
Christian.
> +}
> +EXPORT_SYMBOL(ttm_check_under_lowerlimit);
> +
> static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
> struct ttm_mem_zone *single_zone,
> uint64_t amount, bool reserve)
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index 5edcd89..c9d8903 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
> if (ttm->state != tt_unpopulated)
> return 0;
>
> + if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
> + return -ENOMEM;
> +
> ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> ttm->caching_state);
> if (unlikely(ret != 0)) {
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index b122f6e..37e03d9 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
> if (ttm->state != tt_unpopulated)
> return 0;
>
> + if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
> + return -ENOMEM;
> +
> INIT_LIST_HEAD(&ttm_dma->pages_list);
> i = 0;
>
> diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
> index 8936285..737b5fe 100644
> --- a/include/drm/ttm/ttm_memory.h
> +++ b/include/drm/ttm/ttm_memory.h
> @@ -49,6 +49,8 @@
> * @work: The workqueue callback for the shrink queue.
> * @lock: Lock to protect the @shrink - and the memory accounting members,
> * that is, essentially the whole structure with some exceptions.
> + * @lower_mem_limit: include lower limit of swap space and lower limit of
> + * system memory.
> * @zones: Array of pointers to accounting zones.
> * @num_zones: Number of populated entries in the @zones array.
> * @zone_kernel: Pointer to the kernel zone.
> @@ -67,6 +69,7 @@ struct ttm_mem_global {
> struct workqueue_struct *swap_queue;
> struct work_struct work;
> spinlock_t lock;
> + uint64_t lower_mem_limit;
> struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
> unsigned int num_zones;
> struct ttm_mem_zone *zone_kernel;
> @@ -90,4 +93,6 @@ extern void ttm_mem_global_free_page(struct ttm_mem_global *glob,
> struct page *page, uint64_t size);
> extern size_t ttm_round_pot(size_t size);
> extern uint64_t ttm_get_kernel_zone_memory_size(struct ttm_mem_global *glob);
> +extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
> + uint64_t num_pages, struct ttm_operation_ctx *ctx);
> #endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 11:27 ` Christian König
@ 2018-02-22 11:43 ` He, Roger
2018-02-22 12:54 ` Christian König
2018-02-22 14:05 ` Alex Deucher
0 siblings, 2 replies; 13+ messages in thread
From: He, Roger @ 2018-02-22 11:43 UTC (permalink / raw)
To: Koenig, Christian, dri-devel
-----Original Message-----
From: Koenig, Christian
Sent: Thursday, February 22, 2018 7:28 PM
To: He, Roger <Hongbo.He@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.He@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.):
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?
Thanks
Roger(Hongbo.He)
> ret = ttm_mem_init_kernel_zone(glob, &si);
> if (unlikely(ret != 0))
> goto out_no_zone;
> @@ -469,6 +533,36 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
> }
> EXPORT_SYMBOL(ttm_mem_global_free);
>
> +/*
> + * check if the available mem is under lower memory limit
> + *
> + * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
> + */
> +bool
> +ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
> + uint64_t num_pages,
> + struct ttm_operation_ctx *ctx)
> +{
> + bool ret = false;
> + int64_t available;
> +
> + if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
> + return false;
> +
> + available = get_nr_swap_pages() + si_mem_available();
> + available -= num_pages;
> + if (available < glob->lower_mem_limit)
> + ret = true;
> +
> + return ret;
Don't use a local variable, just use "return true" / "return false;".
Regards,
Christian.
> +}
> +EXPORT_SYMBOL(ttm_check_under_lowerlimit);
> +
> static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
> struct ttm_mem_zone *single_zone,
> uint64_t amount, bool reserve) diff --git
> a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index 5edcd89..c9d8903 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
> if (ttm->state != tt_unpopulated)
> return 0;
>
> + if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
> + return -ENOMEM;
> +
> ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> ttm->caching_state);
> if (unlikely(ret != 0)) {
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index b122f6e..37e03d9 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
> if (ttm->state != tt_unpopulated)
> return 0;
>
> + if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
> + return -ENOMEM;
> +
> INIT_LIST_HEAD(&ttm_dma->pages_list);
> i = 0;
>
> diff --git a/include/drm/ttm/ttm_memory.h
> b/include/drm/ttm/ttm_memory.h index 8936285..737b5fe 100644
> --- a/include/drm/ttm/ttm_memory.h
> +++ b/include/drm/ttm/ttm_memory.h
> @@ -49,6 +49,8 @@
> * @work: The workqueue callback for the shrink queue.
> * @lock: Lock to protect the @shrink - and the memory accounting members,
> * that is, essentially the whole structure with some exceptions.
> + * @lower_mem_limit: include lower limit of swap space and lower
> + limit of
> + * system memory.
> * @zones: Array of pointers to accounting zones.
> * @num_zones: Number of populated entries in the @zones array.
> * @zone_kernel: Pointer to the kernel zone.
> @@ -67,6 +69,7 @@ struct ttm_mem_global {
> struct workqueue_struct *swap_queue;
> struct work_struct work;
> spinlock_t lock;
> + uint64_t lower_mem_limit;
> struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
> unsigned int num_zones;
> struct ttm_mem_zone *zone_kernel;
> @@ -90,4 +93,6 @@ extern void ttm_mem_global_free_page(struct ttm_mem_global *glob,
> struct page *page, uint64_t size);
> extern size_t ttm_round_pot(size_t size);
> extern uint64_t ttm_get_kernel_zone_memory_size(struct
> ttm_mem_global *glob);
> +extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
> + uint64_t num_pages, struct ttm_operation_ctx *ctx);
> #endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 11:43 ` He, Roger
@ 2018-02-22 12:54 ` Christian König
2018-02-23 2:18 ` He, Roger
2018-02-22 14:05 ` Alex Deucher
1 sibling, 1 reply; 13+ messages in thread
From: Christian König @ 2018-02-22 12:54 UTC (permalink / raw)
To: He, Roger, dri-devel
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@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.He@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.
Regards,
Christian.
>
> Thanks
> Roger(Hongbo.He)
>> ret = ttm_mem_init_kernel_zone(glob, &si);
>> if (unlikely(ret != 0))
>> goto out_no_zone;
>> @@ -469,6 +533,36 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
>> }
>> EXPORT_SYMBOL(ttm_mem_global_free);
>>
>> +/*
>> + * check if the available mem is under lower memory limit
>> + *
>> + * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
>> + */
>> +bool
>> +ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages,
>> + struct ttm_operation_ctx *ctx)
>> +{
>> + bool ret = false;
>> + int64_t available;
>> +
>> + if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
>> + return false;
>> +
>> + available = get_nr_swap_pages() + si_mem_available();
>> + available -= num_pages;
>> + if (available < glob->lower_mem_limit)
>> + ret = true;
>> +
>> + return ret;
> Don't use a local variable, just use "return true" / "return false;".
>
> Regards,
> Christian.
>
>> +}
>> +EXPORT_SYMBOL(ttm_check_under_lowerlimit);
>> +
>> static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
>> struct ttm_mem_zone *single_zone,
>> uint64_t amount, bool reserve) diff --git
>> a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> index 5edcd89..c9d8903 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> @@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
>> + return -ENOMEM;
>> +
>> ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
>> ttm->caching_state);
>> if (unlikely(ret != 0)) {
>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> index b122f6e..37e03d9 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> @@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
>> + return -ENOMEM;
>> +
>> INIT_LIST_HEAD(&ttm_dma->pages_list);
>> i = 0;
>>
>> diff --git a/include/drm/ttm/ttm_memory.h
>> b/include/drm/ttm/ttm_memory.h index 8936285..737b5fe 100644
>> --- a/include/drm/ttm/ttm_memory.h
>> +++ b/include/drm/ttm/ttm_memory.h
>> @@ -49,6 +49,8 @@
>> * @work: The workqueue callback for the shrink queue.
>> * @lock: Lock to protect the @shrink - and the memory accounting members,
>> * that is, essentially the whole structure with some exceptions.
>> + * @lower_mem_limit: include lower limit of swap space and lower
>> + limit of
>> + * system memory.
>> * @zones: Array of pointers to accounting zones.
>> * @num_zones: Number of populated entries in the @zones array.
>> * @zone_kernel: Pointer to the kernel zone.
>> @@ -67,6 +69,7 @@ struct ttm_mem_global {
>> struct workqueue_struct *swap_queue;
>> struct work_struct work;
>> spinlock_t lock;
>> + uint64_t lower_mem_limit;
>> struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
>> unsigned int num_zones;
>> struct ttm_mem_zone *zone_kernel;
>> @@ -90,4 +93,6 @@ extern void ttm_mem_global_free_page(struct ttm_mem_global *glob,
>> struct page *page, uint64_t size);
>> extern size_t ttm_round_pot(size_t size);
>> extern uint64_t ttm_get_kernel_zone_memory_size(struct
>> ttm_mem_global *glob);
>> +extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages, struct ttm_operation_ctx *ctx);
>> #endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 11:43 ` He, Roger
2018-02-22 12:54 ` Christian König
@ 2018-02-22 14:05 ` Alex Deucher
2018-02-23 3:46 ` He, Roger
1 sibling, 1 reply; 13+ messages in thread
From: Alex Deucher @ 2018-02-22 14:05 UTC (permalink / raw)
To: He, Roger; +Cc: Koenig, Christian, dri-devel
On Thu, Feb 22, 2018 at 6:43 AM, He, Roger <Hongbo.He@amd.com> wrote:
>
>
> -----Original Message-----
> From: Koenig, Christian
> Sent: Thursday, February 22, 2018 7:28 PM
> To: He, Roger <Hongbo.He@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.He@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.):
Might want to add a comment or use a define for the shift so others
doen't get confused in the future.
Alex
>
> 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?
>
> Thanks
> Roger(Hongbo.He)
>> ret = ttm_mem_init_kernel_zone(glob, &si);
>> if (unlikely(ret != 0))
>> goto out_no_zone;
>> @@ -469,6 +533,36 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
>> }
>> EXPORT_SYMBOL(ttm_mem_global_free);
>>
>> +/*
>> + * check if the available mem is under lower memory limit
>> + *
>> + * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
>> + */
>> +bool
>> +ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages,
>> + struct ttm_operation_ctx *ctx)
>> +{
>> + bool ret = false;
>> + int64_t available;
>> +
>> + if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
>> + return false;
>> +
>> + available = get_nr_swap_pages() + si_mem_available();
>> + available -= num_pages;
>> + if (available < glob->lower_mem_limit)
>> + ret = true;
>> +
>> + return ret;
>
> Don't use a local variable, just use "return true" / "return false;".
>
> Regards,
> Christian.
>
>> +}
>> +EXPORT_SYMBOL(ttm_check_under_lowerlimit);
>> +
>> static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
>> struct ttm_mem_zone *single_zone,
>> uint64_t amount, bool reserve) diff --git
>> a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> index 5edcd89..c9d8903 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> @@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
>> + return -ENOMEM;
>> +
>> ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
>> ttm->caching_state);
>> if (unlikely(ret != 0)) {
>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> index b122f6e..37e03d9 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> @@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
>> + return -ENOMEM;
>> +
>> INIT_LIST_HEAD(&ttm_dma->pages_list);
>> i = 0;
>>
>> diff --git a/include/drm/ttm/ttm_memory.h
>> b/include/drm/ttm/ttm_memory.h index 8936285..737b5fe 100644
>> --- a/include/drm/ttm/ttm_memory.h
>> +++ b/include/drm/ttm/ttm_memory.h
>> @@ -49,6 +49,8 @@
>> * @work: The workqueue callback for the shrink queue.
>> * @lock: Lock to protect the @shrink - and the memory accounting members,
>> * that is, essentially the whole structure with some exceptions.
>> + * @lower_mem_limit: include lower limit of swap space and lower
>> + limit of
>> + * system memory.
>> * @zones: Array of pointers to accounting zones.
>> * @num_zones: Number of populated entries in the @zones array.
>> * @zone_kernel: Pointer to the kernel zone.
>> @@ -67,6 +69,7 @@ struct ttm_mem_global {
>> struct workqueue_struct *swap_queue;
>> struct work_struct work;
>> spinlock_t lock;
>> + uint64_t lower_mem_limit;
>> struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
>> unsigned int num_zones;
>> struct ttm_mem_zone *zone_kernel;
>> @@ -90,4 +93,6 @@ extern void ttm_mem_global_free_page(struct ttm_mem_global *glob,
>> struct page *page, uint64_t size);
>> extern size_t ttm_round_pot(size_t size);
>> extern uint64_t ttm_get_kernel_zone_memory_size(struct
>> ttm_mem_global *glob);
>> +extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages, struct ttm_operation_ctx *ctx);
>> #endif
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 12:54 ` Christian König
@ 2018-02-23 2:18 ` He, Roger
2018-02-23 7:30 ` Christian König
0 siblings, 1 reply; 13+ messages in thread
From: He, Roger @ 2018-02-23 2:18 UTC (permalink / raw)
To: Koenig, Christian, dri-devel
-----Original Message-----
From: Koenig, Christian
Sent: Thursday, February 22, 2018 8:54 PM
To: He, Roger <Hongbo.He@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.He@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.He@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?
Thanks
Roger(Hongbo.He)
>
> Thanks
> Roger(Hongbo.He)
>> ret = ttm_mem_init_kernel_zone(glob, &si);
>> if (unlikely(ret != 0))
>> goto out_no_zone;
>> @@ -469,6 +533,36 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
>> }
>> EXPORT_SYMBOL(ttm_mem_global_free);
>>
>> +/*
>> + * check if the available mem is under lower memory limit
>> + *
>> + * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
>> + */
>> +bool
>> +ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages,
>> + struct ttm_operation_ctx *ctx)
>> +{
>> + bool ret = false;
>> + int64_t available;
>> +
>> + if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
>> + return false;
>> +
>> + available = get_nr_swap_pages() + si_mem_available();
>> + available -= num_pages;
>> + if (available < glob->lower_mem_limit)
>> + ret = true;
>> +
>> + return ret;
> Don't use a local variable, just use "return true" / "return false;".
>
> Regards,
> Christian.
>
>> +}
>> +EXPORT_SYMBOL(ttm_check_under_lowerlimit);
>> +
>> static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
>> struct ttm_mem_zone *single_zone,
>> uint64_t amount, bool reserve) diff --git
>> a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> index 5edcd89..c9d8903 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> @@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
>> + return -ENOMEM;
>> +
>> ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
>> ttm->caching_state);
>> if (unlikely(ret != 0)) {
>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> index b122f6e..37e03d9 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> @@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
>> + return -ENOMEM;
>> +
>> INIT_LIST_HEAD(&ttm_dma->pages_list);
>> i = 0;
>>
>> diff --git a/include/drm/ttm/ttm_memory.h
>> b/include/drm/ttm/ttm_memory.h index 8936285..737b5fe 100644
>> --- a/include/drm/ttm/ttm_memory.h
>> +++ b/include/drm/ttm/ttm_memory.h
>> @@ -49,6 +49,8 @@
>> * @work: The workqueue callback for the shrink queue.
>> * @lock: Lock to protect the @shrink - and the memory accounting members,
>> * that is, essentially the whole structure with some exceptions.
>> + * @lower_mem_limit: include lower limit of swap space and lower
>> + limit of
>> + * system memory.
>> * @zones: Array of pointers to accounting zones.
>> * @num_zones: Number of populated entries in the @zones array.
>> * @zone_kernel: Pointer to the kernel zone.
>> @@ -67,6 +69,7 @@ struct ttm_mem_global {
>> struct workqueue_struct *swap_queue;
>> struct work_struct work;
>> spinlock_t lock;
>> + uint64_t lower_mem_limit;
>> struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
>> unsigned int num_zones;
>> struct ttm_mem_zone *zone_kernel; @@ -90,4 +93,6 @@ extern void
>> ttm_mem_global_free_page(struct ttm_mem_global *glob,
>> struct page *page, uint64_t size);
>> extern size_t ttm_round_pot(size_t size);
>> extern uint64_t ttm_get_kernel_zone_memory_size(struct
>> ttm_mem_global *glob);
>> +extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages, struct ttm_operation_ctx *ctx);
>> #endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 14:05 ` Alex Deucher
@ 2018-02-23 3:46 ` He, Roger
0 siblings, 0 replies; 13+ messages in thread
From: He, Roger @ 2018-02-23 3:46 UTC (permalink / raw)
To: Alex Deucher; +Cc: Koenig, Christian, dri-devel
-----Original Message-----
From: Alex Deucher [mailto:alexdeucher@gmail.com]
Sent: Thursday, February 22, 2018 10:06 PM
To: He, Roger <Hongbo.He@amd.com>
Cc: Koenig, Christian <Christian.Koenig@amd.com>; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
On Thu, Feb 22, 2018 at 6:43 AM, He, Roger <Hongbo.He@amd.com> wrote:
>
>
> -----Original Message-----
> From: Koenig, Christian
> Sent: Thursday, February 22, 2018 7:28 PM
> To: He, Roger <Hongbo.He@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.He@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.):
Might want to add a comment or use a define for the shift so others doen't get confused in the future.
Sure.
Thanks
Roger(Hongbo.He)
>
> 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?
>
> Thanks
> Roger(Hongbo.He)
>> ret = ttm_mem_init_kernel_zone(glob, &si);
>> if (unlikely(ret != 0))
>> goto out_no_zone;
>> @@ -469,6 +533,36 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
>> }
>> EXPORT_SYMBOL(ttm_mem_global_free);
>>
>> +/*
>> + * check if the available mem is under lower memory limit
>> + *
>> + * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
>> + */
>> +bool
>> +ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages,
>> + struct ttm_operation_ctx *ctx) {
>> + bool ret = false;
>> + int64_t available;
>> +
>> + if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
>> + return false;
>> +
>> + available = get_nr_swap_pages() + si_mem_available();
>> + available -= num_pages;
>> + if (available < glob->lower_mem_limit)
>> + ret = true;
>> +
>> + return ret;
>
> Don't use a local variable, just use "return true" / "return false;".
>
> Regards,
> Christian.
>
>> +}
>> +EXPORT_SYMBOL(ttm_check_under_lowerlimit);
>> +
>> static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
>> struct ttm_mem_zone *single_zone,
>> uint64_t amount, bool reserve) diff
>> --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> index 5edcd89..c9d8903 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
>> @@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
>> + return -ENOMEM;
>> +
>> ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
>> ttm->caching_state);
>> if (unlikely(ret != 0)) {
>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> index b122f6e..37e03d9 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> @@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
>> if (ttm->state != tt_unpopulated)
>> return 0;
>>
>> + if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
>> + return -ENOMEM;
>> +
>> INIT_LIST_HEAD(&ttm_dma->pages_list);
>> i = 0;
>>
>> diff --git a/include/drm/ttm/ttm_memory.h
>> b/include/drm/ttm/ttm_memory.h index 8936285..737b5fe 100644
>> --- a/include/drm/ttm/ttm_memory.h
>> +++ b/include/drm/ttm/ttm_memory.h
>> @@ -49,6 +49,8 @@
>> * @work: The workqueue callback for the shrink queue.
>> * @lock: Lock to protect the @shrink - and the memory accounting members,
>> * that is, essentially the whole structure with some exceptions.
>> + * @lower_mem_limit: include lower limit of swap space and lower
>> + limit of
>> + * system memory.
>> * @zones: Array of pointers to accounting zones.
>> * @num_zones: Number of populated entries in the @zones array.
>> * @zone_kernel: Pointer to the kernel zone.
>> @@ -67,6 +69,7 @@ struct ttm_mem_global {
>> struct workqueue_struct *swap_queue;
>> struct work_struct work;
>> spinlock_t lock;
>> + uint64_t lower_mem_limit;
>> struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
>> unsigned int num_zones;
>> struct ttm_mem_zone *zone_kernel; @@ -90,4 +93,6 @@ extern void
>> ttm_mem_global_free_page(struct ttm_mem_global *glob,
>> struct page *page, uint64_t size);
>> extern size_t ttm_round_pot(size_t size);
>> extern uint64_t ttm_get_kernel_zone_memory_size(struct
>> ttm_mem_global *glob);
>> +extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
>> + uint64_t num_pages, struct ttm_operation_ctx
>> +*ctx);
>> #endif
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 10:10 [PATCH] drm/ttm: check if free mem space is under the lower limit Roger He
2018-02-22 11:27 ` Christian König
@ 2018-02-23 5:46 ` kbuild test robot
2018-02-23 8:49 ` kbuild test robot
2 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-02-23 5:46 UTC (permalink / raw)
Cc: Roger He, kbuild-all, dri-devel, Christian.Koenig
[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]
Hi Roger,
I love your patch! Yet something to improve:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.16-rc2 next-20180222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Roger-He/drm-ttm-check-if-free-mem-space-is-under-the-lower-limit/20180223-132039
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-x019-201807 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu//drm/ttm/ttm_memory.c: In function 'ttm_check_under_lowerlimit':
>> drivers/gpu//drm/ttm/ttm_memory.c:554:9: error: 'struct ttm_operation_ctx' has no member named 'flags'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^~
>> drivers/gpu//drm/ttm/ttm_memory.c:554:19: error: 'TTM_OPT_FLAG_FORCE_ALLOC' undeclared (first use in this function); did you mean 'TTM_PAGE_FLAG_ZERO_ALLOC'?
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^~~~~~~~~~~~~~~~~~~~~~~~
TTM_PAGE_FLAG_ZERO_ALLOC
drivers/gpu//drm/ttm/ttm_memory.c:554:19: note: each undeclared identifier is reported only once for each function it appears in
vim +554 drivers/gpu//drm/ttm/ttm_memory.c
535
536 /*
537 * check if the available mem is under lower memory limit
538 *
539 * a. if no swap disk at all or free swap space is under swap_mem_limit
540 * but available system mem is bigger than sys_mem_limit, allow TTM
541 * allocation;
542 *
543 * b. if the available system mem is less than sys_mem_limit but free
544 * swap disk is bigger than swap_mem_limit, allow TTM allocation.
545 */
546 bool
547 ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
548 uint64_t num_pages,
549 struct ttm_operation_ctx *ctx)
550 {
551 bool ret = false;
552 int64_t available;
553
> 554 if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
555 return false;
556
557 available = get_nr_swap_pages() + si_mem_available();
558 available -= num_pages;
559 if (available < glob->lower_mem_limit)
560 ret = true;
561
562 return ret;
563 }
564 EXPORT_SYMBOL(ttm_check_under_lowerlimit);
565
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34131 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-23 2:18 ` He, Roger
@ 2018-02-23 7:30 ` Christian König
0 siblings, 0 replies; 13+ messages in thread
From: Christian König @ 2018-02-23 7:30 UTC (permalink / raw)
To: He, Roger, dri-devel
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@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.He@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.He@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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-22 10:10 [PATCH] drm/ttm: check if free mem space is under the lower limit Roger He
2018-02-22 11:27 ` Christian König
2018-02-23 5:46 ` kbuild test robot
@ 2018-02-23 8:49 ` kbuild test robot
2 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-02-23 8:49 UTC (permalink / raw)
Cc: Roger He, kbuild-all, dri-devel, Christian.Koenig
[-- Attachment #1: Type: text/plain, Size: 4662 bytes --]
Hi Roger,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.16-rc2 next-20180223]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Roger-He/drm-ttm-check-if-free-mem-space-is-under-the-lower-limit/20180223-132039
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-b0-02231423 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/timer.h:5,
from include/linux/workqueue.h:9,
from include/drm/ttm/ttm_memory.h:31,
from drivers/gpu/drm/ttm/ttm_memory.c:30:
drivers/gpu/drm/ttm/ttm_memory.c: In function 'ttm_check_under_lowerlimit':
drivers/gpu/drm/ttm/ttm_memory.c:554:9: error: 'struct ttm_operation_ctx' has no member named 'flags'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> drivers/gpu/drm/ttm/ttm_memory.c:554:2: note: in expansion of macro 'if'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
drivers/gpu/drm/ttm/ttm_memory.c:554:19: error: 'TTM_OPT_FLAG_FORCE_ALLOC' undeclared (first use in this function)
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> drivers/gpu/drm/ttm/ttm_memory.c:554:2: note: in expansion of macro 'if'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
drivers/gpu/drm/ttm/ttm_memory.c:554:19: note: each undeclared identifier is reported only once for each function it appears in
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> drivers/gpu/drm/ttm/ttm_memory.c:554:2: note: in expansion of macro 'if'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
drivers/gpu/drm/ttm/ttm_memory.c:554:9: error: 'struct ttm_operation_ctx' has no member named 'flags'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> drivers/gpu/drm/ttm/ttm_memory.c:554:2: note: in expansion of macro 'if'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
drivers/gpu/drm/ttm/ttm_memory.c:554:9: error: 'struct ttm_operation_ctx' has no member named 'flags'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^
>> drivers/gpu/drm/ttm/ttm_memory.c:554:2: note: in expansion of macro 'if'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^
vim +/if +554 drivers/gpu/drm/ttm/ttm_memory.c
535
536 /*
537 * check if the available mem is under lower memory limit
538 *
539 * a. if no swap disk at all or free swap space is under swap_mem_limit
540 * but available system mem is bigger than sys_mem_limit, allow TTM
541 * allocation;
542 *
543 * b. if the available system mem is less than sys_mem_limit but free
544 * swap disk is bigger than swap_mem_limit, allow TTM allocation.
545 */
546 bool
547 ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
548 uint64_t num_pages,
549 struct ttm_operation_ctx *ctx)
550 {
551 bool ret = false;
552 int64_t available;
553
> 554 if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
555 return false;
556
557 available = get_nr_swap_pages() + si_mem_available();
558 available -= num_pages;
559 if (available < glob->lower_mem_limit)
560 ret = true;
561
562 return ret;
563 }
564 EXPORT_SYMBOL(ttm_check_under_lowerlimit);
565
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24960 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-23 10:19 Roger He
2018-02-23 10:24 ` Christian König
@ 2018-02-24 23:35 ` kbuild test robot
1 sibling, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-02-24 23:35 UTC (permalink / raw)
Cc: Roger He, kbuild-all, dri-devel, Christian.Koenig
[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]
Hi Roger,
I love your patch! Yet something to improve:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.16-rc2 next-20180223]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Roger-He/drm-ttm-check-if-free-mem-space-is-under-the-lower-limit/20180225-033957
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu//drm/ttm/ttm_memory.c: In function 'ttm_check_under_lowerlimit':
>> drivers/gpu//drm/ttm/ttm_memory.c:552:9: error: 'struct ttm_operation_ctx' has no member named 'flags'
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^~
>> drivers/gpu//drm/ttm/ttm_memory.c:552:19: error: 'TTM_OPT_FLAG_FORCE_ALLOC' undeclared (first use in this function); did you mean 'TTM_PAGE_FLAG_ZERO_ALLOC'?
if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
^~~~~~~~~~~~~~~~~~~~~~~~
TTM_PAGE_FLAG_ZERO_ALLOC
drivers/gpu//drm/ttm/ttm_memory.c:552:19: note: each undeclared identifier is reported only once for each function it appears in
vim +552 drivers/gpu//drm/ttm/ttm_memory.c
534
535 /*
536 * check if the available mem is under lower memory limit
537 *
538 * a. if no swap disk at all or free swap space is under swap_mem_limit
539 * but available system mem is bigger than sys_mem_limit, allow TTM
540 * allocation;
541 *
542 * b. if the available system mem is less than sys_mem_limit but free
543 * swap disk is bigger than swap_mem_limit, allow TTM allocation.
544 */
545 bool
546 ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
547 uint64_t num_pages,
548 struct ttm_operation_ctx *ctx)
549 {
550 int64_t available;
551
> 552 if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
553 return false;
554
555 available = get_nr_swap_pages() + si_mem_available();
556 available -= num_pages;
557 if (available < glob->lower_mem_limit)
558 return true;
559
560 return false;
561 }
562 EXPORT_SYMBOL(ttm_check_under_lowerlimit);
563
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40815 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/ttm: check if free mem space is under the lower limit
2018-02-23 10:19 Roger He
@ 2018-02-23 10:24 ` Christian König
2018-02-24 23:35 ` kbuild test robot
1 sibling, 0 replies; 13+ messages in thread
From: Christian König @ 2018-02-23 10:24 UTC (permalink / raw)
To: Roger He, dri-devel
Am 23.02.2018 um 11:19 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
> v6: set lower_mem_limit as 0 to keep original behavior
>
> Signed-off-by: Roger He <Hongbo.He@amd.com>
> ---
> drivers/gpu/drm/ttm/ttm_memory.c | 92 ++++++++++++++++++++++++++++++++
> 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, 103 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
> index aa0c381..0e192f38 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,53 @@ 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);
> + /* convert from 4KB to KB */
> + return snprintf(buffer, PAGE_SIZE, "%llu\n",
> + (unsigned long long) val << 2);
Here we assume that page size is 4KB, better use "val << (PAGE_SHIFT -
10)" and change the comment to something like "convert from number of
pages to KB".
> +}
> +
> +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;
> + /* convert from KB to 4KB */
> + val64 >>= 2;
Dito.
With that fixed the patch is Reviewed-by: Christian König
<christian.koenig@amd.com>.
Thanks,
Christian.
> +
> + 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 +222,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 +435,9 @@ int ttm_mem_global_init(struct ttm_mem_global *glob)
>
> si_meminfo(&si);
>
> + /* set it as 0 by default to keep original behavior of OOM */
> + glob->lower_mem_limit = 0;
> +
> ret = ttm_mem_init_kernel_zone(glob, &si);
> if (unlikely(ret != 0))
> goto out_no_zone;
> @@ -469,6 +532,35 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
> }
> EXPORT_SYMBOL(ttm_mem_global_free);
>
> +/*
> + * check if the available mem is under lower memory limit
> + *
> + * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
> + */
> +bool
> +ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
> + uint64_t num_pages,
> + struct ttm_operation_ctx *ctx)
> +{
> + int64_t available;
> +
> + if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
> + return false;
> +
> + available = get_nr_swap_pages() + si_mem_available();
> + available -= num_pages;
> + if (available < glob->lower_mem_limit)
> + return true;
> +
> + return false;
> +}
> +EXPORT_SYMBOL(ttm_check_under_lowerlimit);
> +
> static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
> struct ttm_mem_zone *single_zone,
> uint64_t amount, bool reserve)
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index 5edcd89..c9d8903 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
> if (ttm->state != tt_unpopulated)
> return 0;
>
> + if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
> + return -ENOMEM;
> +
> ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> ttm->caching_state);
> if (unlikely(ret != 0)) {
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index b122f6e..37e03d9 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
> if (ttm->state != tt_unpopulated)
> return 0;
>
> + if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
> + return -ENOMEM;
> +
> INIT_LIST_HEAD(&ttm_dma->pages_list);
> i = 0;
>
> diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
> index 8936285..737b5fe 100644
> --- a/include/drm/ttm/ttm_memory.h
> +++ b/include/drm/ttm/ttm_memory.h
> @@ -49,6 +49,8 @@
> * @work: The workqueue callback for the shrink queue.
> * @lock: Lock to protect the @shrink - and the memory accounting members,
> * that is, essentially the whole structure with some exceptions.
> + * @lower_mem_limit: include lower limit of swap space and lower limit of
> + * system memory.
> * @zones: Array of pointers to accounting zones.
> * @num_zones: Number of populated entries in the @zones array.
> * @zone_kernel: Pointer to the kernel zone.
> @@ -67,6 +69,7 @@ struct ttm_mem_global {
> struct workqueue_struct *swap_queue;
> struct work_struct work;
> spinlock_t lock;
> + uint64_t lower_mem_limit;
> struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
> unsigned int num_zones;
> struct ttm_mem_zone *zone_kernel;
> @@ -90,4 +93,6 @@ extern void ttm_mem_global_free_page(struct ttm_mem_global *glob,
> struct page *page, uint64_t size);
> extern size_t ttm_round_pot(size_t size);
> extern uint64_t ttm_get_kernel_zone_memory_size(struct ttm_mem_global *glob);
> +extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
> + uint64_t num_pages, struct ttm_operation_ctx *ctx);
> #endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] drm/ttm: check if free mem space is under the lower limit
@ 2018-02-23 10:19 Roger He
2018-02-23 10:24 ` Christian König
2018-02-24 23:35 ` kbuild test robot
0 siblings, 2 replies; 13+ messages in thread
From: Roger He @ 2018-02-23 10:19 UTC (permalink / raw)
To: dri-devel; +Cc: Roger He, Christian.Koenig
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
v6: set lower_mem_limit as 0 to keep original behavior
Signed-off-by: Roger He <Hongbo.He@amd.com>
---
drivers/gpu/drm/ttm/ttm_memory.c | 92 ++++++++++++++++++++++++++++++++
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, 103 insertions(+)
diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
index aa0c381..0e192f38 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,53 @@ 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);
+ /* convert from 4KB to KB */
+ return snprintf(buffer, PAGE_SIZE, "%llu\n",
+ (unsigned long long) val << 2);
+}
+
+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;
+ /* convert from KB to 4KB */
+ val64 >>= 2;
+
+ 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 +222,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 +435,9 @@ int ttm_mem_global_init(struct ttm_mem_global *glob)
si_meminfo(&si);
+ /* set it as 0 by default to keep original behavior of OOM */
+ glob->lower_mem_limit = 0;
+
ret = ttm_mem_init_kernel_zone(glob, &si);
if (unlikely(ret != 0))
goto out_no_zone;
@@ -469,6 +532,35 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
}
EXPORT_SYMBOL(ttm_mem_global_free);
+/*
+ * check if the available mem is under lower memory limit
+ *
+ * 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 disk is bigger than swap_mem_limit, allow TTM allocation.
+ */
+bool
+ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
+ uint64_t num_pages,
+ struct ttm_operation_ctx *ctx)
+{
+ int64_t available;
+
+ if (ctx->flags & TTM_OPT_FLAG_FORCE_ALLOC)
+ return false;
+
+ available = get_nr_swap_pages() + si_mem_available();
+ available -= num_pages;
+ if (available < glob->lower_mem_limit)
+ return true;
+
+ return false;
+}
+EXPORT_SYMBOL(ttm_check_under_lowerlimit);
+
static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
struct ttm_mem_zone *single_zone,
uint64_t amount, bool reserve)
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 5edcd89..c9d8903 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -1100,6 +1100,9 @@ int ttm_pool_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
if (ttm->state != tt_unpopulated)
return 0;
+ if (ttm_check_under_lowerlimit(mem_glob, ttm->num_pages, ctx))
+ return -ENOMEM;
+
ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
ttm->caching_state);
if (unlikely(ret != 0)) {
diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index b122f6e..37e03d9 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -940,6 +940,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev,
if (ttm->state != tt_unpopulated)
return 0;
+ if (ttm_check_under_lowerlimit(mem_glob, num_pages, ctx))
+ return -ENOMEM;
+
INIT_LIST_HEAD(&ttm_dma->pages_list);
i = 0;
diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
index 8936285..737b5fe 100644
--- a/include/drm/ttm/ttm_memory.h
+++ b/include/drm/ttm/ttm_memory.h
@@ -49,6 +49,8 @@
* @work: The workqueue callback for the shrink queue.
* @lock: Lock to protect the @shrink - and the memory accounting members,
* that is, essentially the whole structure with some exceptions.
+ * @lower_mem_limit: include lower limit of swap space and lower limit of
+ * system memory.
* @zones: Array of pointers to accounting zones.
* @num_zones: Number of populated entries in the @zones array.
* @zone_kernel: Pointer to the kernel zone.
@@ -67,6 +69,7 @@ struct ttm_mem_global {
struct workqueue_struct *swap_queue;
struct work_struct work;
spinlock_t lock;
+ uint64_t lower_mem_limit;
struct ttm_mem_zone *zones[TTM_MEM_MAX_ZONES];
unsigned int num_zones;
struct ttm_mem_zone *zone_kernel;
@@ -90,4 +93,6 @@ extern void ttm_mem_global_free_page(struct ttm_mem_global *glob,
struct page *page, uint64_t size);
extern size_t ttm_round_pot(size_t size);
extern uint64_t ttm_get_kernel_zone_memory_size(struct ttm_mem_global *glob);
+extern bool ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
+ uint64_t num_pages, struct ttm_operation_ctx *ctx);
#endif
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-02-24 23:36 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-22 10:10 [PATCH] drm/ttm: check if free mem space is under the lower limit Roger He
2018-02-22 11:27 ` Christian König
2018-02-22 11:43 ` He, Roger
2018-02-22 12:54 ` Christian König
2018-02-23 2:18 ` He, Roger
2018-02-23 7:30 ` Christian König
2018-02-22 14:05 ` Alex Deucher
2018-02-23 3:46 ` He, Roger
2018-02-23 5:46 ` kbuild test robot
2018-02-23 8:49 ` kbuild test robot
2018-02-23 10:19 Roger He
2018-02-23 10:24 ` Christian König
2018-02-24 23:35 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.