Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

35 participants
7098 discussions

Re: [Linaro-mm-sig] memory leak in dma_buf_ioctl

by Steven Rostedt

On Thu, 25 Jul 2019 19:34:01 -0700 syzbot <syzbot+b2098bc44728a4efb3e9(a)syzkaller.appspotmail.com> wrote: > syzbot has bisected this bug to: > > commit 04cf31a759ef575f750a63777cee95500e410994 > Author: Michael Ellerman <mpe(a)ellerman.id.au> > Date: Thu Mar 24 11:04:01 2016 +0000 > > ftrace: Make ftrace_location_range() global It's sad that I have yet to find a single syzbot bisect useful. Really? setting a function from static to global will cause a memory leak in a completely unrelated area of the kernel? I'm about to set these to my /dev/null folder. -- Steve > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=154293f4600000 > start commit: abdfd52a Merge tag 'armsoc-defconfig' of git://git.kernel... > git tree: upstream > final crash: https://syzkaller.appspot.com/x/report.txt?x=174293f4600000 > console output: https://syzkaller.appspot.com/x/log.txt?x=134293f4600000 > kernel config: https://syzkaller.appspot.com/x/.config?x=d31de3d88059b7fa > dashboard link: https://syzkaller.appspot.com/bug?extid=b2098bc44728a4efb3e9 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12526e58600000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=161784f0600000 > > Reported-by: syzbot+b2098bc44728a4efb3e9(a)syzkaller.appspotmail.com > Fixes: 04cf31a759ef ("ftrace: Make ftrace_location_range() global") > > For information about bisection process see: https://goo.gl/tpsmEJ#bisection

6 years, 11 months

Re: [Linaro-mm-sig] Limits for ION Memory Allocator

by Laura Abbott

On 7/17/19 12:31 PM, Alexander Popov wrote: > Hello! > > The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory > Allocator. > > Syzkaller uses several methods [2] to limit memory consumption of the userspace > processes calling the syscalls for testing the kernel: > - setrlimit(), > - cgroups, > - various sysctl. > But these methods don't work for ION Memory Allocator, so any userspace process > that has access to /dev/ion can bring the system to the out-of-memory state. > > An example of a program doing that: > > > #include <sys/types.h> > #include <sys/stat.h> > #include <fcntl.h> > #include <stdio.h> > #include <linux/types.h> > #include <sys/ioctl.h> > > #define ION_IOC_MAGIC 'I' > #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \ > struct ion_allocation_data) > > struct ion_allocation_data { > __u64 len; > __u32 heap_id_mask; > __u32 flags; > __u32 fd; > __u32 unused; > }; > > int main(void) > { > unsigned long i = 0; > int fd = -1; > struct ion_allocation_data data = { > .len = 0x13f65d8c, > .heap_id_mask = 1, > .flags = 0, > .fd = -1, > .unused = 0 > }; > > fd = open("/dev/ion", 0); > if (fd == -1) { > perror("[-] open /dev/ion"); > return 1; > } > > while (1) { > printf("iter %lu\n", i); > ioctl(fd, ION_IOC_ALLOC, &data); > i++; > } > > return 0; > } > > > I looked through the code of ion_alloc() and didn't find any limit checks. > Is it currently possible to limit ION kernel allocations for some process? > > If not, is it a right idea to do that? > Thanks! > Yes, I do think that's the right approach. We're working on moving Ion out of staging and this is something I mentioned to John Stultz. I don't think we've thought too hard about how to do the actual limiting so suggestions are welcome. Thanks, Laura > Best regards, > Alexander > > > [1]: https://github.com/google/syzkaller > [2]: https://github.com/google/syzkaller/blob/master/executor/common_linux.h >

6 years, 11 months

Re: [Linaro-mm-sig] [PATCH 1/2] staging: android: ion: Remove file ion_carveout_heap.c

by Greg KH

On Wed, Jul 03, 2019 at 01:48:41PM +0530, Nishka Dasgupta wrote: > Remove file ion_carveout_heap.c as its functions and definitions are not > used anywhere. > Issue found with Coccinelle. > > Signed-off-by: Nishka Dasgupta <nishkadg.linux(a)gmail.com> > --- > drivers/staging/android/ion/Kconfig | 9 -- > drivers/staging/android/ion/Makefile | 1 - > .../staging/android/ion/ion_carveout_heap.c | 133 ------------------ I keep trying to do this, but others point out that the ion code is "going to be fixed up soon" and that people rely on this interface now. Well, "code outside of the kernel tree" relies on this, which is not ok, but the "soon" people keep insisting on it... Odds are I should just delete all of ION, as there hasn't been any forward progress on it in a long time. Hopefully that wakes some people up... thanks, greg k-h

7 years

Re: [Linaro-mm-sig] [PATCH 2/2] staging: android: ion: Remove file ion_chunk_heap.c

by Laura Abbott

On 7/3/19 4:18 AM, Nishka Dasgupta wrote: > Remove file ion_chunk_heap.c as its functions and definitions are not > used anywhere else. > Issue found with Coccinelle. > Acked-by: Laura Abbott <labbott(a)redhat.com> > Signed-off-by: Nishka Dasgupta <nishkadg.linux(a)gmail.com> > --- > drivers/staging/android/ion/Kconfig | 9 -- > drivers/staging/android/ion/Makefile | 1 - > drivers/staging/android/ion/ion_chunk_heap.c | 147 ------------------- > 3 files changed, 157 deletions(-) > delete mode 100644 drivers/staging/android/ion/ion_chunk_heap.c > > diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig > index dff641451a89..989fe84a9f9d 100644 > --- a/drivers/staging/android/ion/Kconfig > +++ b/drivers/staging/android/ion/Kconfig > @@ -18,15 +18,6 @@ config ION_SYSTEM_HEAP > Choose this option to enable the Ion system heap. The system heap > is backed by pages from the buddy allocator. If in doubt, say Y. > > -config ION_CHUNK_HEAP > - bool "Ion chunk heap support" > - depends on ION > - help > - Choose this option to enable chunk heaps with Ion. This heap is > - similar in function the carveout heap but memory is broken down > - into smaller chunk sizes, typically corresponding to a TLB size. > - Unless you know your system has these regions, you should say N here. > - > config ION_CMA_HEAP > bool "Ion CMA heap support" > depends on ION && DMA_CMA > diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile > index 0ac5465e2841..5f4487b1a224 100644 > --- a/drivers/staging/android/ion/Makefile > +++ b/drivers/staging/android/ion/Makefile > @@ -1,5 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_ION) += ion.o ion_heap.o > obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o > -obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o > obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o > diff --git a/drivers/staging/android/ion/ion_chunk_heap.c b/drivers/staging/android/ion/ion_chunk_heap.c > deleted file mode 100644 > index 1e869f4bad45..000000000000 > --- a/drivers/staging/android/ion/ion_chunk_heap.c > +++ /dev/null > @@ -1,147 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * ION memory allocator chunk heap helper > - * > - * Copyright (C) 2012 Google, Inc. > - */ > - > -#include <linux/dma-mapping.h> > -#include <linux/err.h> > -#include <linux/genalloc.h> > -#include <linux/mm.h> > -#include <linux/scatterlist.h> > -#include <linux/slab.h> > - > -#include "ion.h" > - > -struct ion_chunk_heap { > - struct ion_heap heap; > - struct gen_pool *pool; > - unsigned long chunk_size; > - unsigned long size; > - unsigned long allocated; > -}; > - > -static int ion_chunk_heap_allocate(struct ion_heap *heap, > - struct ion_buffer *buffer, > - unsigned long size, > - unsigned long flags) > -{ > - struct ion_chunk_heap *chunk_heap = > - container_of(heap, struct ion_chunk_heap, heap); > - struct sg_table *table; > - struct scatterlist *sg; > - int ret, i; > - unsigned long num_chunks; > - unsigned long allocated_size; > - > - allocated_size = ALIGN(size, chunk_heap->chunk_size); > - num_chunks = allocated_size / chunk_heap->chunk_size; > - > - if (allocated_size > chunk_heap->size - chunk_heap->allocated) > - return -ENOMEM; > - > - table = kmalloc(sizeof(*table), GFP_KERNEL); > - if (!table) > - return -ENOMEM; > - ret = sg_alloc_table(table, num_chunks, GFP_KERNEL); > - if (ret) { > - kfree(table); > - return ret; > - } > - > - sg = table->sgl; > - for (i = 0; i < num_chunks; i++) { > - unsigned long paddr = gen_pool_alloc(chunk_heap->pool, > - chunk_heap->chunk_size); > - if (!paddr) > - goto err; > - sg_set_page(sg, pfn_to_page(PFN_DOWN(paddr)), > - chunk_heap->chunk_size, 0); > - sg = sg_next(sg); > - } > - > - buffer->sg_table = table; > - chunk_heap->allocated += allocated_size; > - return 0; > -err: > - sg = table->sgl; > - for (i -= 1; i >= 0; i--) { > - gen_pool_free(chunk_heap->pool, page_to_phys(sg_page(sg)), > - sg->length); > - sg = sg_next(sg); > - } > - sg_free_table(table); > - kfree(table); > - return -ENOMEM; > -} > - > -static void ion_chunk_heap_free(struct ion_buffer *buffer) > -{ > - struct ion_heap *heap = buffer->heap; > - struct ion_chunk_heap *chunk_heap = > - container_of(heap, struct ion_chunk_heap, heap); > - struct sg_table *table = buffer->sg_table; > - struct scatterlist *sg; > - int i; > - unsigned long allocated_size; > - > - allocated_size = ALIGN(buffer->size, chunk_heap->chunk_size); > - > - ion_heap_buffer_zero(buffer); > - > - for_each_sg(table->sgl, sg, table->nents, i) { > - gen_pool_free(chunk_heap->pool, page_to_phys(sg_page(sg)), > - sg->length); > - } > - chunk_heap->allocated -= allocated_size; > - sg_free_table(table); > - kfree(table); > -} > - > -static struct ion_heap_ops chunk_heap_ops = { > - .allocate = ion_chunk_heap_allocate, > - .free = ion_chunk_heap_free, > - .map_user = ion_heap_map_user, > - .map_kernel = ion_heap_map_kernel, > - .unmap_kernel = ion_heap_unmap_kernel, > -}; > - > -struct ion_heap *ion_chunk_heap_create(phys_addr_t base, size_t size, > - size_t chunk_size) > -{ > - struct ion_chunk_heap *chunk_heap; > - int ret; > - struct page *page; > - > - page = pfn_to_page(PFN_DOWN(base)); > - ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL)); > - if (ret) > - return ERR_PTR(ret); > - > - chunk_heap = kzalloc(sizeof(*chunk_heap), GFP_KERNEL); > - if (!chunk_heap) > - return ERR_PTR(-ENOMEM); > - > - chunk_heap->chunk_size = chunk_size; > - chunk_heap->pool = gen_pool_create(get_order(chunk_heap->chunk_size) + > - PAGE_SHIFT, -1); > - if (!chunk_heap->pool) { > - ret = -ENOMEM; > - goto error_gen_pool_create; > - } > - chunk_heap->size = size; > - chunk_heap->allocated = 0; > - > - gen_pool_add(chunk_heap->pool, base, size, -1); > - chunk_heap->heap.ops = &chunk_heap_ops; > - chunk_heap->heap.type = ION_HEAP_TYPE_CHUNK; > - chunk_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE; > - pr_debug("%s: base %pa size %zu\n", __func__, &base, size); > - > - return &chunk_heap->heap; > - > -error_gen_pool_create: > - kfree(chunk_heap); > - return ERR_PTR(ret); > -} >

7 years

Re: [Linaro-mm-sig] [PATCH 1/2] staging: android: ion: Remove file ion_carveout_heap.c

by Greg KH

On Wed, Jul 03, 2019 at 02:14:21PM +0530, Nishka Dasgupta wrote: > On 03/07/19 2:07 PM, Greg KH wrote: > > On Wed, Jul 03, 2019 at 01:48:41PM +0530, Nishka Dasgupta wrote: > > > Remove file ion_carveout_heap.c as its functions and definitions are not > > > used anywhere. > > > Issue found with Coccinelle. > > > > > > Signed-off-by: Nishka Dasgupta <nishkadg.linux(a)gmail.com> > > > --- > > > drivers/staging/android/ion/Kconfig | 9 -- > > > drivers/staging/android/ion/Makefile | 1 - > > > .../staging/android/ion/ion_carveout_heap.c | 133 ------------------ > > > > I keep trying to do this, but others point out that the ion code is > > "going to be fixed up soon" and that people rely on this interface now. > > Well, "code outside of the kernel tree" relies on this, which is not ok, > > but the "soon" people keep insisting on it... > > > > Odds are I should just delete all of ION, as there hasn't been any > > forward progress on it in a long time. > > I'm sorry, I don't think I understand. Should I drop these patches from my > tree then? What "tree"? Let's see what the ION maintainer and developers say before rushing to anything. thanks, greg k-h

7 years

Re: [Linaro-mm-sig] [RFC PATCH 0/2] Support for TI Page-based Address Translator

by Robin Murphy

On 07/06/2019 20:35, Andrew F. Davis wrote: > Hello all, > > So I've got a new IP on our new SoC I'm looking to make use of and would > like some help figuring out what framework best matches its function. The > IP is called a "Page-based Address Translator" or PAT. A PAT instance > (there are 5 of these things on our J721e device[0]) is basically a > really simple IOMMU sitting on the interconnect between the device bus > and what is effectively our northbridge called > MSMC (DRAM/SRAM/L3-Cache/Coherency controller). > > Simplified it looks about like this: > > CPUs > | > DRAM --- MSMC --- SRAM/L3 > | > NAVSS - (PATs) > | > --- Device Bus --------- > | | | | > Device Device Device etc.. > > Each PAT has a set a window in high memory (about 0x48_0000_0000 area) > for which any transaction with an address targeting its window will be > routed into that PAT. The PAT then does a simple calculation based on > the how far into the window the address is and the current page size, > does a lookup to an internally held table of translations, then sends the > transaction back out on the interconnect with a new address. Usually this > address should be towards somewhere in DRAM, but can be some other device > or even back into PAT (I'm not sure there is a valid use-case for this > but just a point of interest). > > My gut reaction is that this is an IOMMU which belongs in the IOMMU > subsystem. But there are a couple oddities that make me less sure it is > really suitable for the IOMMU framework. First it doesn't sit in front of > any devices, it sits in front of *all* devices, this means we would have > every device claim it as an IOMMU parent, even though many devices also > have a traditional IOMMU connected. Second, there is only a limited > window of address space per PAT, this means we will get fragmentation and > allocation failures on occasion, in this way it looks to me more like AGP > GART. Third, the window is in high-memory, so unlike some IOMMU devices > which can be used to allow DMA to high-mem from low-mem only devices, PAT > can't be used for that. Lastly it doesn't provide any isolation, if the > access does not target the PAT window it is not used (that is not to say > we don't have isolation, just that it is taken care of by other parts of > the interconnect). > > This means, to me, that PAT has one main purpose: making > physically-contiguous views of scattered pages in system memory for DMA. > But it does that really well, the whole translation table is held in a > PAT-internal SRAM giving 1 bus cycle latency and at full bus bandwidth. > > So what are my options here, is IOMMU the right way to go or not? FWIW, that sounds almost exactly like my (vague) understanding of other GARTs, and as such should be pretty well manageable via the IOMMU API - we already have tegra-gart, for example. The aperture contention issue could certainly be mitigated by letting the firmware claim it's only associated with the display and any other devices which really need it. A further interesting avenue of investigation - now that Christoph's recent work has made it much more possible - would be a second set of IOMMU DMA ops tailored for "GART-like" domains where force_aperture=0, which could behave as dma-direct wherever possible and only use IOMMU remaps when absolutely necessary. Robin. > Looking around the kernel I also see the char dev ARP/GART interface > which looks like a good fit, but also looks quite dated and my guess > deprecated at this point. Moving right along.. > > Another thing I saw is we have the support upstream of the DMM device[1] > available in some OMAPx/AM57x SoCs. I'm a little more familiar with this > device. The DMM is a bundle of IPs and in fact one of them is called > "PAT" and it even does basically the same thing this incarnation of "PAT" > does. It's upstream integration design is a bit questionable > unfortunately, the DMM support was integrated into the OMAPDRM display > driver, which does make some sense then given its support for rotation > (using TILER IP contained in DMM). The issue with this was that the > DMM/TILER/PAT IP was not part of the our display IP, but instead out at > the end of the shared device bus, inside the external memory controller. > Like this new PAT this meant that any IP that could make use of it, but > only the display framework could actually provide buffers backed by it. > This meant, for instance, if we wanted to decode some video buffer using > our video decoder we would have to allocate from DRM framework then pass > that over to the V4L2 system. This doesn't make much sense and required > the user-space to know about this odd situation and allocate from the > right spot or else have to use up valuable CMA space or waste memory with > dedicated carveouts. > > Another idea would be to have this as a special central allocator > (exposed through DMA-BUF heaps[2] or ION) that would give out normal > system memory as a DMA-BUF but remap it with PAT if a device that only > supports contiguous memory tries to attach/map that DMA-BUF. > > One last option would be to allow user-space to choose to make the buffer > contiguous when it needs. That's what the driver in this series allows. > We expose a remapping device, user-space passes it a non-contiguous > DMA-BUF handle and it passes a contiguous one back. Simple as that. > > So how do we use this, lets take Android for example, we don't know at > allocation time if a rendering buffer will end up going back into the GPU > for further processing, or if it will be consumed directly by the display. > This is a problem for us as our GPU can work with non-contiguous buffers > but our display cannot, so any buffers that could possibly go to the > display at some point currently needs to be allocated as contiguous from > the start, this leads to a lot of unneeded use of carveout/CMA memory. > With this driver on the other hand, we allocate regular non-contiguous > system memory (again using DMA-BUF heaps, but ION could work here too), > then only when a buffer is about to be sent to the display we pass the > handle to this DMA-BUF to our driver here and take the handle it gives > back and pass that to the display instead. > > As said, it is probably not the ideal solution but it does work and was > used for some early testing of the IP. > > Well, sorry for the wall of text. > Any and all suggestions very welcome and appreciated. > > Thanks, > Andrew > > [0] http://www.ti.com/lit/pdf/spruil1 > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri… > [2] https://lkml.org/lkml/2019/6/6/1211 > > Andrew F. Davis (2): > dt-bindings: soc: ti: Add TI PAT bindings > soc: ti: Add Support for the TI Page-based Address Translator (PAT) > > .../devicetree/bindings/misc/ti,pat.txt | 34 ++ > drivers/soc/ti/Kconfig | 9 + > drivers/soc/ti/Makefile | 1 + > drivers/soc/ti/ti-pat.c | 569 ++++++++++++++++++ > include/uapi/linux/ti-pat.h | 44 ++ > 5 files changed, 657 insertions(+) > create mode 100644 Documentation/devicetree/bindings/misc/ti,pat.txt > create mode 100644 drivers/soc/ti/ti-pat.c > create mode 100644 include/uapi/linux/ti-pat.h >

7 years

Re: [Linaro-mm-sig] [PATCH] staging: android: fix style problem

by Greg KH

On Sat, Jun 15, 2019 at 01:11:56AM +0530, Saiyam Doshi wrote: > checkpatch reported "WARNING: line over 80 characters". This > patch fixes it by aligning function arguments. > > Signed-off-by: Saiyam Doshi <saiyamdoshi.in(a)gmail.com> > --- > drivers/staging/android/ion/ion_chunk_heap.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/ion_chunk_heap.c b/drivers/staging/android/ion/ion_chunk_heap.c > index 3cdde9c1a717..6aceab2e77e4 100644 > --- a/drivers/staging/android/ion/ion_chunk_heap.c > +++ b/drivers/staging/android/ion/ion_chunk_heap.c > @@ -107,7 +107,9 @@ static struct ion_heap_ops chunk_heap_ops = { > .unmap_kernel = ion_heap_unmap_kernel, > }; > > -struct ion_heap *ion_chunk_heap_create(phys_addr_t base, size_t size, size_t chunk_size) > +struct ion_heap *ion_chunk_heap_create(phys_addr_t base, > + size_t size, > + size_t chunk_size) Just break the line at "size_t size,", no need for 3 lines when 2 will do. thanks, greg k-h

7 years

Re: [Linaro-mm-sig] [PATCH] dma-buf: refcount the attachment for cache_sgt_mapping

by Koenig, Christian

Am 12.06.19 um 10:15 schrieb Nicolin Chen: > Hi Christian, > > On Wed, Jun 12, 2019 at 08:05:53AM +0000, Koenig, Christian wrote: >> Am 12.06.19 um 10:02 schrieb Nicolin Chen: >> [SNIP] >>> We haven't used DRM/GRM_PRIME yet but I am also curious would it >>> benefit DRM also if we reduce this overhead in the dma_buf? >> No, not at all. > From you replies, in a summary, does it means that there won't be a case > of DRM having a dma_buf attaching to the same device, i.e. multiple calls > of drm_gem_prime_import() function with same parameters of dev + dma_buf? Well, there are some cases where this happens. But in those cases we intentionally want to get a new attachment :) So thinking more about it you would actually break those and that is not something we can do. > If so, we can just ignore/drop this patch. Sorry for the misunderstanding. It might be interesting for things like P2P, but even then it might be better to just cache the P2P settings instead of the full attachment. Regards, Christian. > > Thanks > Nicolin

7 years

Re: [Linaro-mm-sig] [PATCH] dma-buf: refcount the attachment for cache_sgt_mapping

by Koenig, Christian

Am 12.06.19 um 10:02 schrieb Nicolin Chen: > Hi Christian, > > Thanks for the quick reply. > > On Wed, Jun 12, 2019 at 07:45:38AM +0000, Koenig, Christian wrote: >> Am 12.06.19 um 03:22 schrieb Nicolin Chen: >>> Commit f13e143e7444 ("dma-buf: start caching of sg_table objects v2") >>> added a support of caching the sgt pointer into an attach pointer to >>> let users reuse the sgt pointer without another mapping. However, it >>> might not totally work as most of dma-buf callers are doing attach() >>> and map_attachment() back-to-back, using drm_prime.c for example: >>> drm_gem_prime_import_dev() { >>> attach = dma_buf_attach() { >>> /* Allocating a new attach */ >>> attach = kzalloc(); >>> /* .... */ >>> return attach; >>> } >>> dma_buf_map_attachment(attach, direction) { >>> /* attach->sgt would be always empty as attach is new */ >>> if (attach->sgt) { >>> /* Reuse attach->sgt */ >>> } >>> /* Otherwise, map it */ >>> attach->sgt = map(); >>> } >>> } >>> >>> So, for a cache_sgt_mapping use case, it would need to get the same >>> attachment pointer in order to reuse its sgt pointer. So this patch >>> adds a refcount to the attach() function and lets it search for the >>> existing attach pointer by matching the dev pointer. >> I don't think that this is a good idea. >> >> We use sgt caching as workaround for locking order problems and want to >> remove it again in the long term. > Oh. I thought it was for a performance improving purpose. It may > be a misunderstanding then. > >> So what is the actual use case of this? > We have some similar downstream changes at dma_buf to reduce the > overhead from multiple clients of the same device doing attach() > and map_attachment() calls for the same dma_buf. I don't think that this is a good idea over all. A driver calling attach for the same buffer is doing something wrong in the first place and we should not work around this in the DMA-buf handling. > We haven't used DRM/GRM_PRIME yet but I am also curious would it > benefit DRM also if we reduce this overhead in the dma_buf? No, not at all. Regards, Christian. > > Thanks > Nicolin

7 years

Re: [Linaro-mm-sig] [PATCH] dma-buf: refcount the attachment for cache_sgt_mapping

by Koenig, Christian

Am 12.06.19 um 03:22 schrieb Nicolin Chen: > Commit f13e143e7444 ("dma-buf: start caching of sg_table objects v2") > added a support of caching the sgt pointer into an attach pointer to > let users reuse the sgt pointer without another mapping. However, it > might not totally work as most of dma-buf callers are doing attach() > and map_attachment() back-to-back, using drm_prime.c for example: > drm_gem_prime_import_dev() { > attach = dma_buf_attach() { > /* Allocating a new attach */ > attach = kzalloc(); > /* .... */ > return attach; > } > dma_buf_map_attachment(attach, direction) { > /* attach->sgt would be always empty as attach is new */ > if (attach->sgt) { > /* Reuse attach->sgt */ > } > /* Otherwise, map it */ > attach->sgt = map(); > } > } > > So, for a cache_sgt_mapping use case, it would need to get the same > attachment pointer in order to reuse its sgt pointer. So this patch > adds a refcount to the attach() function and lets it search for the > existing attach pointer by matching the dev pointer. I don't think that this is a good idea. We use sgt caching as workaround for locking order problems and want to remove it again in the long term. So what is the actual use case of this? Regards, Christian. > > Signed-off-by: Nicolin Chen <nicoleotsuka(a)gmail.com> > --- > drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++++++ > include/linux/dma-buf.h | 2 ++ > 2 files changed, 25 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index f4104a21b069..d0260553a31c 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -559,6 +559,21 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, > if (WARN_ON(!dmabuf || !dev)) > return ERR_PTR(-EINVAL); > > + /* cache_sgt_mapping requires to reuse the same attachment pointer */ > + if (dmabuf->ops->cache_sgt_mapping) { > + mutex_lock(&dmabuf->lock); > + > + /* Search for existing attachment and increase its refcount */ > + list_for_each_entry(attach, &dmabuf->attachments, node) { > + if (dev != attach->dev) > + continue; > + atomic_inc_not_zero(&attach->refcount); > + goto unlock_attach; > + } > + > + mutex_unlock(&dmabuf->lock); > + } > + > attach = kzalloc(sizeof(*attach), GFP_KERNEL); > if (!attach) > return ERR_PTR(-ENOMEM); > @@ -575,6 +590,9 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, > } > list_add(&attach->node, &dmabuf->attachments); > > + atomic_set(&attach->refcount, 1); > + > +unlock_attach: > mutex_unlock(&dmabuf->lock); > > return attach; > @@ -599,6 +617,11 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > if (WARN_ON(!dmabuf || !attach)) > return; > > + /* Decrease the refcount for cache_sgt_mapping use cases */ > + if (dmabuf->ops->cache_sgt_mapping && > + atomic_dec_return(&attach->refcount)) > + return; > + > if (attach->sgt) > dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index 8a327566d7f4..65f12212ca2e 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -333,6 +333,7 @@ struct dma_buf { > * @dev: device attached to the buffer. > * @node: list of dma_buf_attachment. > * @sgt: cached mapping. > + * @refcount: refcount of the attachment for the same device. > * @dir: direction of cached mapping. > * @priv: exporter specific attachment data. > * > @@ -350,6 +351,7 @@ struct dma_buf_attachment { > struct device *dev; > struct list_head node; > struct sg_table *sgt; > + atomic_t refcount; > enum dma_data_direction dir; > void *priv; > };

7 years

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig