Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

8 participants
2915 discussions

Re: [Linaro-mm-sig] [PATCH v4 04/13] drm/shmem-helper: Provide a vmap function for short-term mappings

by Daniel Vetter

On Wed, Jan 27, 2021 at 01:08:05PM +0100, Thomas Zimmermann wrote: > Hi > > Am 11.01.21 um 17:50 schrieb Daniel Vetter: > > On Fri, Jan 08, 2021 at 10:43:31AM +0100, Thomas Zimmermann wrote: > > > Implementations of the vmap/vunmap GEM callbacks may perform pinning > > > of the BO and may acquire the associated reservation object's lock. > > > Callers that only require a mapping of the contained memory can thus > > > interfere with other tasks that require exact pinning, such as scanout. > > > This is less of an issue with private SHMEM buffers, but may happen > > > with imported ones. > > > > > > Therefore provide the new interfaces drm_gem_shmem_vmap_local() and > > > drm_gem_shmem_vunmap_local(), which only perform the vmap/vunmap > > > operations. Callers have to hold the reservation lock while the mapping > > > persists. > > > > > > This patch also connects GEM SHMEM helpers to GEM object functions with > > > equivalent functionality. > > > > > > v4: > > > * call dma_buf_{vmap,vunmap}_local() where necessary (Daniel) > > > * move driver changes into separate patches (Daniel) > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > > > --- > > > drivers/gpu/drm/drm_gem_shmem_helper.c | 90 +++++++++++++++++++++++--- > > > include/drm/drm_gem_shmem_helper.h | 2 + > > > 2 files changed, 84 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > > index 9825c378dfa6..298832b2b43b 100644 > > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > > @@ -32,6 +32,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = { > > > .get_sg_table = drm_gem_shmem_get_sg_table, > > > .vmap = drm_gem_shmem_vmap, > > > .vunmap = drm_gem_shmem_vunmap, > > > + .vmap_local = drm_gem_shmem_vmap_local, > > > + .vunmap_local = drm_gem_shmem_vunmap_local, > > > .mmap = drm_gem_shmem_mmap, > > > }; > > > @@ -261,7 +263,8 @@ void drm_gem_shmem_unpin(struct drm_gem_object *obj) > > > } > > > EXPORT_SYMBOL(drm_gem_shmem_unpin); > > > -static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map) > > > +static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map, > > > + bool local) > > > > This is a bit spaghetti and also has the problem that we're not changing > > shmem->vmap_use_count under different locks, depending upon which path > > we're taking. > > > > I think the cleanest would be if we pull the if (import_attach) case out > > of the _locked() version completely, for all cases, and also outside of > > the shmem->vmap_lock. This means no caching of vmaps in the shmem layer > > anymore for imported buffers, but this is no longer a problem: We cache > > them in the exporters instead (I think at least, if not maybe need to fix > > that where it's expensive). > > There's no vmap refcounting in amdgpu AFAICT. So importing pages from there > into an SHMEM object has the potential of breaking. IIRC same fro radeon and > nouveau. As long as the pinning is refcounted I think it should be fine, it's just that if you have multiple vmaps (e.g. 2 udl devices plugged in) we'll set up 2 vmaps. Which is a point pointless, but not really harmful. At least on 64bit where there's enough virtual address space. > So I'm somewhat reluctant to making this change. I guess I'll look elsewhere > first to fix some of the locking issues (e.g., my recent ast cursor > patches). If this would break for amdgpu/radeon/nouveau then we already have a bug, since 2 udl devices can provoke this issue already as-is. So I don't think this should be a blocker. -Daniel > > Best regards > Thomas > > > > > Other option would be to unly pull it out for the _vmap_local case, but > > that's a bit ugly because no longer symmetrical in the various paths. > > > > > { > > > struct drm_gem_object *obj = &shmem->base; > > > int ret = 0; > > > @@ -272,7 +275,10 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct > > > } > > > if (obj->import_attach) { > > > - ret = dma_buf_vmap(obj->import_attach->dmabuf, map); > > > + if (local) > > > + ret = dma_buf_vmap_local(obj->import_attach->dmabuf, map); > > > + else > > > + ret = dma_buf_vmap(obj->import_attach->dmabuf, map); > > > if (!ret) { > > > if (WARN_ON(map->is_iomem)) { > > > ret = -EIO; > > > @@ -313,7 +319,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct > > > return ret; > > > } > > > -/* > > > +/** > > > * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object > > > * @shmem: shmem GEM object > > > * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > > > @@ -339,15 +345,53 @@ int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) > > > ret = mutex_lock_interruptible(&shmem->vmap_lock); > > > if (ret) > > > return ret; > > > - ret = drm_gem_shmem_vmap_locked(shmem, map); > > > + ret = drm_gem_shmem_vmap_locked(shmem, map, false); > > > mutex_unlock(&shmem->vmap_lock); > > > return ret; > > > } > > > EXPORT_SYMBOL(drm_gem_shmem_vmap); > > > +/** > > > + * drm_gem_shmem_vmap_local - Create a virtual mapping for a shmem GEM object > > > + * @shmem: shmem GEM object > > > + * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > > > + * store. > > > + * > > > + * This function makes sure that a contiguous kernel virtual address mapping > > > + * exists for the buffer backing the shmem GEM object. > > > + * > > > + * The function is called with the BO's reservation object locked. Callers must > > > + * hold the lock until after unmapping the buffer. > > > + * > > > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. But > > > + * it can also be called by drivers directly, in which case it will hide the > > > + * differences between dma-buf imported and natively allocated objects. > > > > So for the other callbacks I tried to make sure we have different entry > > points for this, since it's not really the same thing and because of the > > locking mess we have with dma_resv_lock vs various pre-existing local > > locking scheme, it's easy to get a mess. > > > > I think the super clean version here would be to also export just the > > internal stuff for the ->v(un)map_local hooks, but that's maybe a bit too > > much boilerplate for no real gain. > > -Daniel > > > > > + * > > > + * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap_local(). > > > + * > > > + * Returns: > > > + * 0 on success or a negative error code on failure. > > > + */ > > > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > > > +{ > > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > > + int ret; > > > + > > > + dma_resv_assert_held(obj->resv); > > > + > > > + ret = mutex_lock_interruptible(&shmem->vmap_lock); > > > + if (ret) > > > + return ret; > > > + ret = drm_gem_shmem_vmap_locked(shmem, map, true); > > > + mutex_unlock(&shmem->vmap_lock); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL(drm_gem_shmem_vmap_local); > > > + > > > static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > > > - struct dma_buf_map *map) > > > + struct dma_buf_map *map, bool local) > > > { > > > struct drm_gem_object *obj = &shmem->base; > > > @@ -358,7 +402,10 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > > > return; > > > if (obj->import_attach) > > > - dma_buf_vunmap(obj->import_attach->dmabuf, map); > > > + if (local) > > > + dma_buf_vunmap_local(obj->import_attach->dmabuf, map); > > > + else > > > + dma_buf_vunmap(obj->import_attach->dmabuf, map); > > > else > > > vunmap(shmem->vaddr); > > > @@ -366,7 +413,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > > > drm_gem_shmem_put_pages(shmem); > > > } > > > -/* > > > +/** > > > * drm_gem_shmem_vunmap - Unmap a virtual mapping fo a shmem GEM object > > > * @shmem: shmem GEM object > > > * @map: Kernel virtual address where the SHMEM GEM object was mapped > > > @@ -384,11 +431,38 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) > > > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > > mutex_lock(&shmem->vmap_lock); > > > - drm_gem_shmem_vunmap_locked(shmem, map); > > > + drm_gem_shmem_vunmap_locked(shmem, map, false); > > > mutex_unlock(&shmem->vmap_lock); > > > } > > > EXPORT_SYMBOL(drm_gem_shmem_vunmap); > > > +/** > > > + * drm_gem_shmem_vunmap_local - Unmap a virtual mapping fo a shmem GEM object > > > + * @shmem: shmem GEM object > > > + * @map: Kernel virtual address where the SHMEM GEM object was mapped > > > + * > > > + * This function cleans up a kernel virtual address mapping acquired by > > > + * drm_gem_shmem_vmap_local(). The mapping is only removed when the use count > > > + * drops to zero. > > > + * > > > + * The function is called with the BO's reservation object locked. > > > + * > > > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. > > > + * But it can also be called by drivers directly, in which case it will hide > > > + * the differences between dma-buf imported and natively allocated objects. > > > + */ > > > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > > > +{ > > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > > + > > > + dma_resv_assert_held(obj->resv); > > > + > > > + mutex_lock(&shmem->vmap_lock); > > > + drm_gem_shmem_vunmap_locked(shmem, map, true); > > > + mutex_unlock(&shmem->vmap_lock); > > > +} > > > +EXPORT_SYMBOL(drm_gem_shmem_vunmap_local); > > > + > > > struct drm_gem_shmem_object * > > > drm_gem_shmem_create_with_handle(struct drm_file *file_priv, > > > struct drm_device *dev, size_t size, > > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > > > index 434328d8a0d9..3f59bdf749aa 100644 > > > --- a/include/drm/drm_gem_shmem_helper.h > > > +++ b/include/drm/drm_gem_shmem_helper.h > > > @@ -114,7 +114,9 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem); > > > int drm_gem_shmem_pin(struct drm_gem_object *obj); > > > void drm_gem_shmem_unpin(struct drm_gem_object *obj); > > > int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map); > > > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > > > void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map); > > > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > > > int drm_gem_shmem_madvise(struct drm_gem_object *obj, int madv); > > > -- > > > 2.29.2 > > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v3] dmabuf: Add the capability to expose DMA-BUF stats in sysfs

by Sumit Semwal

Hi Simon, On Thu, 28 Jan 2021 at 20:01, Simon Ser <contact(a)emersion.fr> wrote: > > On Thursday, January 28th, 2021 at 1:03 PM, Sumit Semwal <sumit.semwal(a)linaro.org> wrote: > > > Since he didn't comment over Hridya's last clarification about the > > tracepoints to track total GPU memory allocations being orthogonal to > > this series, I assumed he agreed with it. > > IIRC he's away this week. (I don't remember when he comes back.) > > > Daniel, do you still have objections around adding this patch in? > > (Adding him explicitly in CC) Thanks for doing this! Best, Sumit.

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH 08/29] dma-buf: Avoid comma separated statements

by Christian König

Am 31.01.21 um 18:39 schrieb Joe Perches: > On Wed, 2021-02-03 at 14:26 +0100, Christian König wrote: >> Am 30.01.21 um 19:47 schrieb Joe Perches: >>> On Mon, 2020-08-24 at 21:56 -0700, Joe Perches wrote: >>>> Use semicolons and braces. >>> Ping? >>>> Signed-off-by: Joe Perches <joe(a)perches.com> >> Reviewed-by: Christian König <christian.koenig(a)amd.com> >> >> Do you have commit rights to drm-misc-next? > No. Pushed. Thanks for the help, Christian. > >>>> --- >>>> drivers/dma-buf/st-dma-fence.c | 7 +++++-- >>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c >>>> index e593064341c8..c8a12d7ad71a 100644 >>>> --- a/drivers/dma-buf/st-dma-fence.c >>>> +++ b/drivers/dma-buf/st-dma-fence.c >>>> @@ -471,8 +471,11 @@ static int thread_signal_callback(void *arg) >>>> dma_fence_signal(f1); >>>> >>>> smp_store_mb(cb.seen, false); >>>> - if (!f2 || dma_fence_add_callback(f2, &cb.cb, simple_callback)) >>>> - miss++, cb.seen = true; >>>> + if (!f2 || >>>> + dma_fence_add_callback(f2, &cb.cb, simple_callback)) { >>>> + miss++; >>>> + cb.seen = true; >>>> + } >>>> >>>> if (!t->before) >>>> dma_fence_signal(f1); >

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v4 2/4] mm: failfast mode with __GFP_NORETRY in alloc_contig_range

by Minchan Kim

On Thu, Jan 28, 2021 at 08:53:25AM +0100, Michal Hocko wrote: > On Wed 27-01-21 12:42:45, Minchan Kim wrote: > > On Tue, Jan 26, 2021 at 08:44:49AM +0100, Michal Hocko wrote: > > > On Mon 25-01-21 11:33:36, Minchan Kim wrote: > > > > On Mon, Jan 25, 2021 at 02:12:00PM +0100, Michal Hocko wrote: > > > > > On Thu 21-01-21 09:55:00, Minchan Kim wrote: > > > > > > Contiguous memory allocation can be stalled due to waiting > > > > > > on page writeback and/or page lock which causes unpredictable > > > > > > delay. It's a unavoidable cost for the requestor to get *big* > > > > > > contiguous memory but it's expensive for *small* contiguous > > > > > > memory(e.g., order-4) because caller could retry the request > > > > > > in different range where would have easy migratable pages > > > > > > without stalling. > > > > > > > > > > > > This patch introduce __GFP_NORETRY as compaction gfp_mask in > > > > > > alloc_contig_range so it will fail fast without blocking > > > > > > when it encounters pages needed waiting. > > > > > > > > > > I am not against controling how hard this allocator tries with gfp mask > > > > > but this changelog is rather void on any data and any user. > > > > > > > > > > It is also rather dubious to have retries when then caller says to not > > > > > retry. > > > > > > > > Since max_tries is 1 with ++tries, it shouldn't retry. > > > > > > OK, I have missed that. This is a tricky code. ASYNC mode should be > > > completely orthogonal to the retries count. Those are different things. > > > Page allocator does an explicit bail out based on __GFP_NORETRY. You > > > should be doing the same. > > > > Before sending next revision, let me check this part again. > > > > I want to use __GFP_NORETRY to indicate "opportunistic-easy-to-fail attempt" > > and I want to use ASYNC migrate_mode to help the goal. > > > > Do you see the problem? > > No, as I've said. This is a normal NORETRY policy. And ASYNC migration > is a mere implementation detail you do not have bother your users about. > This is the semantic view. From the implementation POV it should be the > gfp mask to drive decisions rather than a random (ASYNC) flag to control > retries as you did here. Make sense. Let me cook next revision. Thanks for the review, Michal.

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v4 2/4] mm: failfast mode with __GFP_NORETRY in alloc_contig_range

by Minchan Kim

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v4 1/4] mm: cma: introduce gfp flag in cma_alloc instead of no_warn

by Minchan Kim

On Tue, Jan 26, 2021 at 08:38:08AM +0100, Michal Hocko wrote: > On Mon 25-01-21 11:42:34, Minchan Kim wrote: > > On Mon, Jan 25, 2021 at 02:07:01PM +0100, Michal Hocko wrote: > > > On Thu 21-01-21 09:54:59, Minchan Kim wrote: > > > > The upcoming patch will introduce __GFP_NORETRY semantic > > > > in alloc_contig_range which is a failfast mode of the API. > > > > Instead of adding a additional parameter for gfp, replace > > > > no_warn with gfp flag. > > > > > > > > To keep old behaviors, it follows the rule below. > > > > > > > > no_warn gfp_flags > > > > > > > > false GFP_KERNEL > > > > true GFP_KERNEL|__GFP_NOWARN > > > > gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN) > > > > > > > > Reviewed-by: Suren Baghdasaryan <surenb(a)google.com> > > > > Signed-off-by: Minchan Kim <minchan(a)kernel.org> > > > [...] > > > > diff --git a/mm/cma.c b/mm/cma.c > > > > index 0ba69cd16aeb..d50627686fec 100644 > > > > --- a/mm/cma.c > > > > +++ b/mm/cma.c > > > > @@ -419,13 +419,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { } > > > > * @cma: Contiguous memory region for which the allocation is performed. > > > > * @count: Requested number of pages. > > > > * @align: Requested alignment of pages (in PAGE_SIZE order). > > > > - * @no_warn: Avoid printing message about failed allocation > > > > + * @gfp_mask: GFP mask to use during the cma allocation. > > > > > > Call out supported gfp flags explicitly. Have a look at kvmalloc_node > > > for a guidance. > > > > How about this? > > > > > > diff --git a/mm/cma.c b/mm/cma.c > > index d50627686fec..b94727b694d6 100644 > > --- a/mm/cma.c > > +++ b/mm/cma.c > > @@ -423,6 +423,10 @@ static inline void cma_debug_show_areas(struct cma *cma) { } > > * > > * This function allocates part of contiguous memory on specific > > * contiguous memory area. > > + * > > + * For gfp_mask, GFP_KERNEL and __GFP_NORETRY are supported. __GFP_NORETRY > > + * will avoid costly functions(e.g., waiting on page_writeback and locking) > > + * at current implementaion during the page migration. > > rather than explicitly mentioning what the flag implies I think it would > be more useful to state the intended usecase. See how kvmalloc_node says > "__GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is > preferable to the vmalloc fallback, due to visible performance > drawbacks. > __GFP_NOWARN is also supported to suppress allocation failure messages." > > This would help people not familiar with internals to see whether this > flag is a good fit for them. > > In this case I woul go with > " > @flags: gfp mask. Must be compatible (superset) with GFP_KERNEL. > [...] > Reclaim modifiers (__GFP_RETRY_MAYFAIL, __GFP_NOFAIL) are not supported. > __GFP_NORETRY is supported, and it should be used for opportunistic > allocation attempts that should rather fail quickly when the caller has > a fallback strategy. > " > > Obviously for this patch you will go with a simple statement that > Reclaim modifiers are not supported at all. After more discussion for gfp_flags in thread of next patch, let me changes a bit more based on it. Thanks for the suggestion, Michal.

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v4 4/4] dma-buf: heaps: add chunk heap to dmabuf heaps

by Minchan Kim

On Tue, Jan 26, 2021 at 08:46:05AM +0100, Michal Hocko wrote: > On Thu 21-01-21 09:55:02, Minchan Kim wrote: > > From: Hyesoo Yu <hyesoo.yu(a)samsung.com> > > > > This patch supports chunk heap that allocates the buffers that > > arranged into a list a fixed size chunks taken from CMA. > > > > The chunk heap driver is bound directly to a reserved_memory > > node by following Rob Herring's suggestion in [1]. > > > > [1] https://lore.kernel.org/lkml/20191025225009.50305-2-john.stultz@linaro.org/… > > Who is using this allocator in the kernel? Userspace uses the memory via mapping it via dmabuf.

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v4 2/4] mm: failfast mode with __GFP_NORETRY in alloc_contig_range

by Minchan Kim

On Tue, Jan 26, 2021 at 08:44:49AM +0100, Michal Hocko wrote: > On Mon 25-01-21 11:33:36, Minchan Kim wrote: > > On Mon, Jan 25, 2021 at 02:12:00PM +0100, Michal Hocko wrote: > > > On Thu 21-01-21 09:55:00, Minchan Kim wrote: > > > > Contiguous memory allocation can be stalled due to waiting > > > > on page writeback and/or page lock which causes unpredictable > > > > delay. It's a unavoidable cost for the requestor to get *big* > > > > contiguous memory but it's expensive for *small* contiguous > > > > memory(e.g., order-4) because caller could retry the request > > > > in different range where would have easy migratable pages > > > > without stalling. > > > > > > > > This patch introduce __GFP_NORETRY as compaction gfp_mask in > > > > alloc_contig_range so it will fail fast without blocking > > > > when it encounters pages needed waiting. > > > > > > I am not against controling how hard this allocator tries with gfp mask > > > but this changelog is rather void on any data and any user. > > > > > > It is also rather dubious to have retries when then caller says to not > > > retry. > > > > Since max_tries is 1 with ++tries, it shouldn't retry. > > OK, I have missed that. This is a tricky code. ASYNC mode should be > completely orthogonal to the retries count. Those are different things. > Page allocator does an explicit bail out based on __GFP_NORETRY. You > should be doing the same. A concern with __GFP_NOWAIT is regardless of flags passed to cma_alloc, internal implementation of alloc_contig_range inside will use blockable operation. See __alloc_contig_migrate_range. If we go with __GFP_NOWAIT, we should propagate the gfp_mask inside of __alloc_contig_migrate_range to make cma_alloc consistent with alloc_pages. (IIUC, that's what you want - make gfp_mask consistent between cma_alloc and alloc_pages) but I am worry about the direction will make complicate situation since cma invovles migration context as well as target page allocation context. Sometime, the single gfp flag could be trouble to express both contexts all at once. > > > > > > > Also why didn't you consider GFP_NOWAIT semantic for non blocking mode? > > > > GFP_NOWAIT seems to be low(specific) flags rather than the one I want to > > express. Even though I said only page writeback/lock in the description, > > the goal is to avoid costly operations we might find later so such > > "failfast", I thought GFP_NORETRY would be good fit. > > I suspect you are too focused on implementation details here. Think > about the indended semantic. Callers of this functionality will not > think about those (I hope because if they rely on these details then the > whole thing will become unmaintainable because any change would require > an audit of all existing users). All you should be caring about is to > control how expensive the call can be. GFP_NOWAIT is not really low > level from that POV. It gives you a very lightweight non-sleeping > attempt to allocate. GFP_NORETRY will give you potentially sleeping but > an opportunistic-easy-to-fail attempt. And so on. See how that is > absolutely free of any page writeback or any specific locking. With above reason I mentioned, I wanted to express __GFP_NORETRY as "opportunistic-easy-to-fail attempt" to support cma_alloc as "failfast" for migration context. > -- > Michal Hocko > SUSE Labs

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v4 1/4] mm: cma: introduce gfp flag in cma_alloc instead of no_warn

by Minchan Kim

On Mon, Jan 25, 2021 at 02:07:01PM +0100, Michal Hocko wrote: > On Thu 21-01-21 09:54:59, Minchan Kim wrote: > > The upcoming patch will introduce __GFP_NORETRY semantic > > in alloc_contig_range which is a failfast mode of the API. > > Instead of adding a additional parameter for gfp, replace > > no_warn with gfp flag. > > > > To keep old behaviors, it follows the rule below. > > > > no_warn gfp_flags > > > > false GFP_KERNEL > > true GFP_KERNEL|__GFP_NOWARN > > gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN) > > > > Reviewed-by: Suren Baghdasaryan <surenb(a)google.com> > > Signed-off-by: Minchan Kim <minchan(a)kernel.org> > [...] > > diff --git a/mm/cma.c b/mm/cma.c > > index 0ba69cd16aeb..d50627686fec 100644 > > --- a/mm/cma.c > > +++ b/mm/cma.c > > @@ -419,13 +419,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { } > > * @cma: Contiguous memory region for which the allocation is performed. > > * @count: Requested number of pages. > > * @align: Requested alignment of pages (in PAGE_SIZE order). > > - * @no_warn: Avoid printing message about failed allocation > > + * @gfp_mask: GFP mask to use during the cma allocation. > > Call out supported gfp flags explicitly. Have a look at kvmalloc_node > for a guidance. How about this? diff --git a/mm/cma.c b/mm/cma.c index d50627686fec..b94727b694d6 100644 --- a/mm/cma.c +++ b/mm/cma.c @@ -423,6 +423,10 @@ static inline void cma_debug_show_areas(struct cma *cma) { } * * This function allocates part of contiguous memory on specific * contiguous memory area. + * + * For gfp_mask, GFP_KERNEL and __GFP_NORETRY are supported. __GFP_NORETRY + * will avoid costly functions(e.g., waiting on page_writeback and locking) + * at current implementaion during the page migration. */ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align, gfp_t gfp_mask)

4 years, 5 months

Re: [Linaro-mm-sig] [PATCH v4 2/4] mm: failfast mode with __GFP_NORETRY in alloc_contig_range

by Minchan Kim

On Mon, Jan 25, 2021 at 02:12:00PM +0100, Michal Hocko wrote: > On Thu 21-01-21 09:55:00, Minchan Kim wrote: > > Contiguous memory allocation can be stalled due to waiting > > on page writeback and/or page lock which causes unpredictable > > delay. It's a unavoidable cost for the requestor to get *big* > > contiguous memory but it's expensive for *small* contiguous > > memory(e.g., order-4) because caller could retry the request > > in different range where would have easy migratable pages > > without stalling. > > > > This patch introduce __GFP_NORETRY as compaction gfp_mask in > > alloc_contig_range so it will fail fast without blocking > > when it encounters pages needed waiting. > > I am not against controling how hard this allocator tries with gfp mask > but this changelog is rather void on any data and any user. > > It is also rather dubious to have retries when then caller says to not > retry. Since max_tries is 1 with ++tries, it shouldn't retry. > > Also why didn't you consider GFP_NOWAIT semantic for non blocking mode? GFP_NOWAIT seems to be low(specific) flags rather than the one I want to express. Even though I said only page writeback/lock in the description, the goal is to avoid costly operations we might find later so such "failfast", I thought GFP_NORETRY would be good fit. > > > Signed-off-by: Minchan Kim <minchan(a)kernel.org> > > --- > > mm/page_alloc.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index b031a5ae0bd5..1cdc3ee0b22e 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -8491,12 +8491,16 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > > unsigned int nr_reclaimed; > > unsigned long pfn = start; > > unsigned int tries = 0; > > + unsigned int max_tries = 5; > > int ret = 0; > > struct migration_target_control mtc = { > > .nid = zone_to_nid(cc->zone), > > .gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_RETRY_MAYFAIL, > > }; > > > > + if (cc->alloc_contig && cc->mode == MIGRATE_ASYNC) > > + max_tries = 1; > > + > > migrate_prep(); > > > > while (pfn < end || !list_empty(&cc->migratepages)) { > > @@ -8513,7 +8517,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > > break; > > } > > tries = 0; > > - } else if (++tries == 5) { > > + } else if (++tries == max_tries) { > > ret = ret < 0 ? ret : -EBUSY; > > break; > > } > > @@ -8564,7 +8568,7 @@ int alloc_contig_range(unsigned long start, unsigned long end, > > .nr_migratepages = 0, > > .order = -1, > > .zone = page_zone(pfn_to_page(start)), > > - .mode = MIGRATE_SYNC, > > + .mode = gfp_mask & __GFP_NORETRY ? MIGRATE_ASYNC : MIGRATE_SYNC, > > .ignore_skip_hint = true, > > .no_set_skip_hint = true, > > .gfp_mask = current_gfp_context(gfp_mask), > > -- > > 2.30.0.296.g2bfb1c46d8-goog > > -- > Michal Hocko > SUSE Labs

4 years, 5 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig