Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

24 participants
7063 discussions

Re: [PATCH 2/3] dma-buf: sort fences in dma_fence_unwrap_merge

by Christian König

Am 30.10.24 um 19:10 schrieb Friedrich Vock: > On 24.10.24 14:41, Christian König wrote: >> The merge function initially handled only individual fences and >> arrays which in turn were created by the merge function. This allowed >> to create the new array by a simple merge sort based on the fence >> context number. >> >> The problem is now that since the addition of timeline sync objects >> userspace can create chain containers in basically any fence context >> order. >> >> If those are merged together it can happen that we create really >> large arrays since the merge sort algorithm doesn't work any more. >> >> So put an insert sort behind the merge sort which kicks in when the >> input fences are not in the expected order. This isn't as efficient >> as a heap sort, but has better properties for the most common use >> case. >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/dma-buf/dma-fence-unwrap.c | 39 ++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-fence-unwrap.c >> b/drivers/dma-buf/dma-fence-unwrap.c >> index 628af51c81af..d9aa280d9ff6 100644 >> --- a/drivers/dma-buf/dma-fence-unwrap.c >> +++ b/drivers/dma-buf/dma-fence-unwrap.c >> @@ -106,7 +106,7 @@ struct dma_fence >> *__dma_fence_unwrap_merge(unsigned int num_fences, >> fences[i] = dma_fence_unwrap_first(fences[i], &iter[i]); >> >> count = 0; >> - do { >> + while (true) { >> unsigned int sel; >> >> restart: >> @@ -144,11 +144,40 @@ struct dma_fence >> *__dma_fence_unwrap_merge(unsigned int num_fences, >> } >> } >> >> - if (tmp) { >> - array[count++] = dma_fence_get(tmp); >> - fences[sel] = dma_fence_unwrap_next(&iter[sel]); >> + if (!tmp) >> + break; >> + >> + /* >> + * We could use a binary search here, but since the assumption >> + * is that the main input are already sorted dma_fence_arrays >> + * just looking from end has a higher chance of finding the >> + * right location on the first try >> + */ >> + >> + for (i = count; i--;) { > > This is broken. The first iteration of this loop will always index out > of bounds. Nope, that is correct. The condition is evaluated before the loop, so the i-- reduces the index to the last element in the array. Regards, Christian. > What you probably want here is: > > + for (i = count - 1; count && i--;) { > > This intentionally overflows for count == 0, but the ++i after the loop > undoes that. Maybe it would be worth a comment to point out that's > intentional. > >> + if (likely(array[i]->context < tmp->context)) >> + break; >> + >> + if (array[i]->context == tmp->context) { >> + if (dma_fence_is_later(tmp, array[i])) { >> + dma_fence_put(array[i]); >> + array[i] = dma_fence_get(tmp); >> + } >> + fences[sel] = dma_fence_unwrap_next(&iter[sel]); >> + goto restart; >> + } >> } >> - } while (tmp); >> + >> + ++i; >> + /* >> + * Make room for the fence, this should be a nop most of the >> + * time. >> + */ >> + memcpy(&array[i + 1], &array[i], (count - i) * sizeof(*array)); > > Need memmove here, src and dst alias. > > I took it for a spin with these things fixed and it seemed to resolve > the issue as well. How do you want to proceed? I guess I would be > comfortable putting a Reviewed-by and/or Tested-by on a version with > these things fixed (with the usual caveat that I'm not a maintainer - I > guess the process requires (at least one) reviewer to be?). > > By the way, I guess you might've had some internal branches where this > fix needed to go into quick or something? Usually I'm happy to make a v2 > for my patches myself, too ;) > > Regards, > Friedrich > >> + array[i] = dma_fence_get(tmp); >> + fences[sel] = dma_fence_unwrap_next(&iter[sel]); >> + count++; >> + }; >> >> if (count == 0) { >> tmp = dma_fence_allocate_private_stub(ktime_get()); >

1 year, 9 months

Re: Requirements to merge new heaps in the kernel

by Al Viro

On Thu, Oct 31, 2024 at 05:45:23PM +0100, Maxime Ripard wrote: > On Wed, Oct 30, 2024 at 12:16:22PM +0100, metux wrote: > > On 22.10.24 10:38, Maxime Ripard wrote: > > > I'm still interested in merging a carve-out driver[1], since it seems to be > > > in every vendor BSP and got asked again last week. > > > > > > I remember from our discussion that for new heap types to be merged, we > > > needed a kernel use-case. Looking back, I'm not entirely sure how one > > > can provide that given that heaps are essentially facilities for > > > user-space. > > > > For those who didn't follow your work, could you please give a short > > intro what's that all about ? > > > > If I understand you correctly, you'd like the infrastructure of > > kmalloc() et al for things / memory regions that aren't the usual heap, > > right ? > > No, not really. The discussion is about dma-buf heaps. They allow to > allocate buffers suitable for DMA from userspace. It might or might not > from the system memory, at the heap driver discretion. I'm afraid you've misinterpreted that - our hexapedal friend had just * noticed that excessive crossposting can get it banned * got itself a new address * posted a solitary ping as the first test * followed that by testing the ability to cross-post (posting you'd been replying to, contents on chatGPT level) * proceeded to use its shiny new address for more of the chorus whinge exercise they'd been holding with several other similar fellows (or sock puppets, for all I know). Just ignore the wanker - it wasn't trying to get any information other than "will the posting get through" anyway.

1 year, 9 months

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Tue, Oct 29, 2024 at 04:35:16PM +0000, Pavel Begunkov wrote: > I see, the reply is about your phrase about additional memory > abstractions: > > "... don't really need to build memory buffer abstraction over > memory buffer abstraction." Yes, over the exsting memory buffer abstraction (dma_buf). > If you mean internals, making up a dmabuf that has never existed in the > picture in the first place is not cleaner or easier in any way. If that > changes, e.g. there is more code to reuse in the future, we can unify it > then. I'm not sure what "making up" means here, they are all made up :) > > with pre-registering the memry with the iommu to get good performance > > in IOMMU-enabled setups. > > The page pool already does that just like it handles the normal > path without providers. In which case is basically is a dma-buf. If you'd expose it as such we could actually use to communicate between subsystems in the kernel.

1 year, 9 months

Re: [PATCH 1/3] dma-buf/dma-fence_array: use kvzalloc

by Christian König

Am 25.10.24 um 08:52 schrieb Friedrich Vock: > On 24.10.24 22:29, Matthew Brost wrote: >> On Thu, Oct 24, 2024 at 02:41:57PM +0200, Christian König wrote: >>> Reports indicates that some userspace applications try to merge more >>> than >>> 80k of fences into a single dma_fence_array leading to a warning from >> >> Really, yikes. > > Not really IME. Unless Christian means some reports I don't have access > to, the cases where userspace applications tried to do that were really > just cases where the fence count exploded exponentially because > dma_fence_unwrap_merge failed to actually merge identical fences (see > patch 2). At no point have I actually seen apps trying to merge 80k+ > unique fences. While working on it I've modified our stress test tool to send the same 1GiB SDMA copy to 100k different contexts. Turned out it's perfectly possible to create so many fences, there is nothing blocking userspace to do it. While this isn't a realistic use case the kernel should at least not crash or spill a warning, but either handle or reject it gracefully. Friedrich can you confirm that patch two in this series fixes the problem? I would really like to get it into drm-misc-fixes before 6.12 comes out. Thanks, Christian. > > Regards, > Friedrich > >> >>> kzalloc() that the requested size becomes to big. >>> >>> While that is clearly an userspace bug we should probably handle >>> that case >>> gracefully in the kernel. >>> >>> So we can either reject requests to merge more than a reasonable >>> amount of >>> fences (64k maybe?) or we can start to use kvzalloc() instead of >>> kzalloc(). >>> This patch here does the later. >>> >> >> This patch seems reasonable to me if the above use is in fact valid. >> >>> Signed-off-by: Christian König <christian.koenig(a)amd.com> >>> CC: stable(a)vger.kernel.org >> >> Fixes tag? >> >> Patch itself LGTM: >> Reviewed-by: Matthew Brost <matthew.brost(a)intel.com> >> >>> --- >>> drivers/dma-buf/dma-fence-array.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence-array.c >>> b/drivers/dma-buf/dma-fence-array.c >>> index 8a08ffde31e7..46ac42bcfac0 100644 >>> --- a/drivers/dma-buf/dma-fence-array.c >>> +++ b/drivers/dma-buf/dma-fence-array.c >>> @@ -119,8 +119,8 @@ static void dma_fence_array_release(struct >>> dma_fence *fence) >>> for (i = 0; i < array->num_fences; ++i) >>> dma_fence_put(array->fences[i]); >>> >>> - kfree(array->fences); >>> - dma_fence_free(fence); >>> + kvfree(array->fences); >>> + kvfree_rcu(fence, rcu); >>> } >>> >>> static void dma_fence_array_set_deadline(struct dma_fence *fence, >>> @@ -153,7 +153,7 @@ struct dma_fence_array >>> *dma_fence_array_alloc(int num_fences) >>> { >>> struct dma_fence_array *array; >>> >>> - return kzalloc(struct_size(array, callbacks, num_fences), >>> GFP_KERNEL); >>> + return kvzalloc(struct_size(array, callbacks, num_fences), >>> GFP_KERNEL); >>> } >>> EXPORT_SYMBOL(dma_fence_array_alloc); >>> >>> -- >>> 2.34.1 >>> >

1 year, 9 months

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Thu, Oct 24, 2024 at 05:40:02PM +0100, Pavel Begunkov wrote: > On 10/24/24 17:06, Christoph Hellwig wrote: > > On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote: > > > > That's not what this series does. It adds the new memory_provider_ops > > > > set of hooks, with once implementation for dmabufs, and one for > > > > io_uring zero copy. > > > > > > First, it's not a _new_ abstraction over a buffer as you called it > > > before, the abstraction (net_iov) is already merged. > > > > Umm, it is a new ops vector. > > I don't understand what you mean. Callback? struct memory_provider_ops. It's a method table or ops vetor, no callbacks involved. > Then please go ahead and take a look at the patchset in question > and see how much of dmabuf handling is there comparing to pure > networking changes. The point that it's a new set of API and lots > of changes not related directly to dmabufs stand. dmabufs is useful > there as an abstraction there, but it's a very long stretch saying > that the series is all about it. I did take a look, that's why I replied. > > > on an existing network specific abstraction, which are not restricted to > > > pages or anything specific in the long run, but the flow of which from > > > net stack to user and back is controlled by io_uring. If you worry about > > > abuse, io_uring can't even sanely initialise those buffers itself and > > > therefore asking the page pool code to do that. > > > > No, I worry about trying to io_uring for not good reason. This > > It sounds that the argument is that you just don't want any > io_uring APIs, I don't think you'd be able to help you with > that. No, that's complete misinterpreting what I'm saying. Of course an io_uring API is fine. But tying low-level implementation details to to is not. > > pre-cludes in-kernel uses which would be extremly useful for > > Uses of what? devmem TCP is merged, I'm not removing it, > and the net_iov abstraction is in there, which can be potentially > be reused by other in-kernel users if that'd even make sense. How when you are hardcoding io uring memory registrations instead of making them a generic dmabuf? Which btw would also really help with pre-registering the memry with the iommu to get good performance in IOMMU-enabled setups.

1 year, 9 months

Important Message: Incoming mails have been suspended in your email server !!!

by Server Administrator lists.linaro.org

1 year, 9 months

Re: [PATCH 1/3] dma-buf/dma-fence_array: use kvzalloc

by Christian König

Am 25.10.24 um 11:05 schrieb Tvrtko Ursulin: > > On 25/10/2024 09:59, Tvrtko Ursulin wrote: >> >> On 24/10/2024 13:41, Christian König wrote: >>> Reports indicates that some userspace applications try to merge more >>> than >>> 80k of fences into a single dma_fence_array leading to a warning from >>> kzalloc() that the requested size becomes to big. >>> >>> While that is clearly an userspace bug we should probably handle >>> that case >>> gracefully in the kernel. >>> >>> So we can either reject requests to merge more than a reasonable >>> amount of >>> fences (64k maybe?) or we can start to use kvzalloc() instead of >>> kzalloc(). >>> This patch here does the later. >> >> Rejecting would potentially be safer, otherwise there is a path for >> userspace to trigger a warn in kvmalloc_node (see 0829b5bcdd3b >> ("drm/i915: 2 GiB of relocations ought to be enough for anybody*")) >> and spam dmesg at will. > > Actually that is a WARN_ON_*ONCE* there so maybe not so critical to > invent a limit. Up for discussion I suppose. > > Regards, > > Tvrtko > >> >> Question is what limit to set... That's one of the reasons why I opted for kvzalloc() initially. I mean we could use some nice round number like 65536, but that would be totally arbitrary. Any comments on the other two patches? I need to get them upstream. Thanks, Christian. >> >> Regards, >> >> Tvrtko >> >>> Signed-off-by: Christian König <christian.koenig(a)amd.com> >>> CC: stable(a)vger.kernel.org >>> --- >>> drivers/dma-buf/dma-fence-array.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence-array.c >>> b/drivers/dma-buf/dma-fence-array.c >>> index 8a08ffde31e7..46ac42bcfac0 100644 >>> --- a/drivers/dma-buf/dma-fence-array.c >>> +++ b/drivers/dma-buf/dma-fence-array.c >>> @@ -119,8 +119,8 @@ static void dma_fence_array_release(struct >>> dma_fence *fence) >>> for (i = 0; i < array->num_fences; ++i) >>> dma_fence_put(array->fences[i]); >>> - kfree(array->fences); >>> - dma_fence_free(fence); >>> + kvfree(array->fences); >>> + kvfree_rcu(fence, rcu); >>> } >>> static void dma_fence_array_set_deadline(struct dma_fence *fence, >>> @@ -153,7 +153,7 @@ struct dma_fence_array >>> *dma_fence_array_alloc(int num_fences) >>> { >>> struct dma_fence_array *array; >>> - return kzalloc(struct_size(array, callbacks, num_fences), >>> GFP_KERNEL); >>> + return kvzalloc(struct_size(array, callbacks, num_fences), >>> GFP_KERNEL); >>> } >>> EXPORT_SYMBOL(dma_fence_array_alloc);

1 year, 9 months

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote: > > That's not what this series does. It adds the new memory_provider_ops > > set of hooks, with once implementation for dmabufs, and one for > > io_uring zero copy. > > First, it's not a _new_ abstraction over a buffer as you called it > before, the abstraction (net_iov) is already merged. Umm, it is a new ops vector. > Second, you mention devmem TCP, and it's not just a page pool with > "dmabufs", it's a user API to use it and other memory agnostic > allocation logic. And yes, dmabufs there is the least technically > important part. Just having a dmabuf handle solves absolutely nothing. It solves a lot, becaue it provides a proper abstraction. > > So you are precluding zero copy RX into anything but your magic > > io_uring buffers, and using an odd abstraction for that. > > Right io_uring zero copy RX API expects transfer to happen into io_uring > controlled buffers, and that's the entire idea. Buffers that are based > on an existing network specific abstraction, which are not restricted to > pages or anything specific in the long run, but the flow of which from > net stack to user and back is controlled by io_uring. If you worry about > abuse, io_uring can't even sanely initialise those buffers itself and > therefore asking the page pool code to do that. No, I worry about trying to io_uring for not good reason. This pre-cludes in-kernel uses which would be extremly useful for network storage drivers, and it precludes device memory of all kinds. > I'm even more confused how that would help. The user API has to > be implemented and adding a new dmabuf gives nothing, not even > mentioning it's not clear what semantics of that beast is > supposed to be. > The dma-buf maintainers already explained to you last time that there is absolutely no need to use the dmabuf UAPI, you can use dma-bufs through in-kernel interfaces just fine.

1 year, 10 months

Fix Fix fence merge handling

by Christian König

Hi guys, turned out that userspace can also merge dma_fence_chain contains which can result in really huge arrays. Fix those merges to sort the arrays and remove the duplicates. Additional to that start to use kvzalloc() for dma_fence_array containers so that can handle much larger arrays if necessary. Please review and comment, Christian.

1 year, 10 months

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Wed, Oct 23, 2024 at 03:34:53PM +0100, Pavel Begunkov wrote: > It doesn't care much what kind of memory it is, nor it's important > for internals how it's imported, it's user addresses -> pages for > user convenience sake. All the net_iov setup code is in the page pool > core code. What it does, however, is implementing the user API, so That's not what this series does. It adds the new memory_provider_ops set of hooks, with once implementation for dmabufs, and one for io_uring zero copy. So you are precluding zero copy RX into anything but your magic io_uring buffers, and using an odd abstraction for that. The right way would be to support zero copy RX into every designated dmabuf, and make io_uring work with udmabuf or if absolutely needed it's own kind of dmabuf. Instead we create a maze of incompatible abstractions here. The use case of e.g. doing zero copy receive into a NVMe CMB using PCIe P2P transactions is every but made up, so this does create a problem.

1 year, 10 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig