-----Original Message----- From: Alex Deucher [mailto:alexdeucher@xxxxxxxxx] Sent: Thursday, February 22, 2018 10:06 PM To: He, Roger <Hongbo.He@xxxxxxx> Cc: Koenig, Christian <Christian.Koenig@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx 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@xxxxxxx> wrote: > > > -----Original Message----- > From: Koenig, Christian > Sent: Thursday, February 22, 2018 7:28 PM > To: He, Roger <Hongbo.He@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx > 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@xxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
