Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

8 participants
2915 discussions

Re: Tackling the indefinite/user DMA fence problem

by Daniel Stone

Hi, On Wed, 25 May 2022 at 15:07, Simon Ser <contact(a)emersion.fr> wrote: > On Wednesday, May 25th, 2022 at 15:51, Daniel Vetter <daniel(a)ffwll.ch> wrote: > > > You can add that to the list of reasons why compositors need to stop > > > using buffers with unsignaled fences. There's plenty of other reasons > > > there already (the big one being that otherwise slow clients can slow > > > down the compositor, even if the compositor uses a high priority context > > > and the HW supports preemption). > > > > > > Yeah that's tbh another reason why I think we shouldn't do umf as a > > transparent thing - compositors need to get better anyway, so we might as > > well take this as a chance to do this right. > > As a compositor dev, I agree -- we should definitely be smarter about > this. Note, it would help a lot to have a good way to integrate the > waits into a poll(2) event loop. The same holds for Weston. We're currently working through a bunch of internal infrastructure to be able to handle this. Mutter (aka GNOME) is also really well-placed to be able to do this. Having pollable waits would be really useful, but I don't think it's essential. In my strawman I'm just waking up at the usual just-before-repaint point and checking; if it doesn't make it for this frame then we'll wait for the next frame. If someone submits buffers which take 4 repaint periods to clear then we'll have 3 'unnecessary' wakeups, but given the GPU is already slammed then it's not an efficiency problem I don't think. (I don't know if all the other compositor people share this view.) Cheers, Daniel

3 years, 1 month

Re: Tackling the indefinite/user DMA fence problem

by Daniel Vetter

On Wed, May 25, 2022 at 03:28:41PM +0200, Michel Dänzer wrote: > On 2022-05-25 15:05, Daniel Vetter wrote: > > On Tue, May 17, 2022 at 12:28:17PM +0200, Christian König wrote: > >> Am 09.05.22 um 16:10 schrieb Daniel Vetter: > >>> On Mon, May 09, 2022 at 08:56:41AM +0200, Christian König wrote: > >>>> Am 04.05.22 um 12:08 schrieb Daniel Vetter: > >>>>> > >>>>> If the goal is specifically atomic kms, then there's an entire can of > >>>>> worms there that I really don't want to think about, but it exists: We > >>>>> have dma_fence as out-fences from atomic commit, and that's already > >>>>> massively broken since most drivers allocate some memory or at least take > >>>>> locks which can allocate memory in their commit path. Like i2c. Putting a > >>>>> userspace memory fence as in-fence in there makes that problem > >>>>> substantially worse, since at least in theory you're just not allowed to > >>>>> might_faul in atomic_commit_tail. > >>>> Yes, that's unfortunately one of the goals as well and yes I completely > >>>> agree on the can of worms. But I think I've solved that. > >>>> > >>>> What I do in the patch set is to enforce that the out fence is an user fence > >>>> when the driver supports user in fences as well. > >>>> > >>>> Since user fences doesn't have the memory management dependency drivers can > >>>> actually allocate memory or call I2C functions which takes locks which have > >>>> memory allocation dependencies. > >>>> > >>>> Or do I miss some other reason why you can't fault or allocate memory in > >>>> atomic_commit_tail? At least lockdep seems to be happy about that now. > >>> The problem is a bit that this breaks the uapi already. At least if the > >>> goal is to have this all be perfectly transparent for userspace - as you > >>> as you have multi-gpu setups going on at least. > >> > >> Question here is why do you think there is an UAPI break? We currently wait > >> in a work item already, so where exactly is the problem? > > > > It's a bit washy, but dma_fence and hence implicit sync is supposed to > > finish in finite time. umf just doesn't. > > > > Ofc in reality you can still flood your compositor and they're not very > > robust, but with umf it's trivial to just hang your compositor forever and > > nothing happens. > > You can add that to the list of reasons why compositors need to stop > using buffers with unsignaled fences. There's plenty of other reasons > there already (the big one being that otherwise slow clients can slow > down the compositor, even if the compositor uses a high priority context > and the HW supports preemption). Yeah that's tbh another reason why I think we shouldn't do umf as a transparent thing - compositors need to get better anyway, so we might as well take this as a chance to do this right. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 1 month

Tackling the indefinite/user DMA fence problem

by Christian König

Hello everyone, it's a well known problem that the DMA-buf subsystem mixed synchronization and memory management requirements into the same dma_fence and dma_resv objects. Because of this dma_fence objects need to guarantee that they complete within a finite amount of time or otherwise the system can easily deadlock. One of the few good things about this problem is that it is really good understood by now. Daniel and others came up with some documentation: https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_… And Jason did an excellent presentation about that problem on last years LPC: https://lpc.events/event/11/contributions/1115/ Based on that we had been able to reject new implementations of infinite/user DMA fences and mitigate the effect of the few existing ones. The still remaining down side is that we don't have a way of using user fences as dependency in both the explicit (sync_file, drm_syncobj) as well as the implicit (dma_resv) synchronization objects, resulting in numerous problems and limitations for things like HMM, user queues etc.... This patch set here now tries to tackle this problem by untangling the synchronization from the memory management. What it does *not* try to do is to fix the existing kernel fences, because I think we now can all agree on that this isn't really possible. To archive this goal what I do in this patch set is to add some parallel infrastructure to cleanly separate normal kernel dma_fence objects from indefinite/user fences: 1. It introduce a DMA_FENCE_FLAG_USER define (after renaming some existing driver defines). To note that a certain dma_fence is an user fence and *must* be ignore by memory management and never used as dependency for normal none user dma_fence objects. 2. The dma_fence_array and dma_fence_chain containers are modified so that they are marked as user fences whenever any of their contained fences are an user fence. 3. The dma_resv object gets a new DMA_RESV_USAGE_USER flag which must be used with indefinite/user fences and separates those into it's own synchronization domain. 4. The existing dma_buf_poll_add_cb() function is modified so that indefinite/user fences are included in the polling. 5. The sync_file synchronization object is modified so that we essentially have two fence streams instead of just one. 6. The drm_syncobj is modified in a similar way. User fences are just ignored unless the driver explicitly states support to wait for them. 7. The DRM subsystem gains a new DRIVER_USER_FENCE flag which drivers can use to indicate the need for user fences. If user fences are used the atomic mode setting starts to support user fences as IN/OUT fences. 8. Lockdep is used at various critical locations to ensure that nobody ever tries to mix user fences with non user fences. The general approach is to just ignore user fences unless a driver stated explicitely support for them. On top of all of this I've hacked amdgpu so that we add the resulting CS fence only as kernel dependency to the dma_resv object and an additional wrapped up with a dma_fence_array and a stub user fence. The result is that the newly added atomic modeset functions now correctly wait for the user fence to complete before doing the flip. And dependent CS don't pipeline any more, but rather block on the CPU before submitting work. After tons of debugging and testing everything now seems to not go up in flames immediately and even lockdep is happy with the annotations. I'm perfectly aware that this is probably by far the most controversial patch set I've ever created and I really wish we wouldn't need it. But we certainly have the requirement for this and I don't see much other chance to get that working in an UAPI compatible way. Thoughts/comments? Regards, Christian.

3 years, 1 month

[PATCH v4 0/3] add Aspeed udc driver for ast2600

by Neal Liu

This patch series aim to add Aspeed USB 2.0 Device Controller (udc) driver, including driver itself, device tree node and documentation. Change since v3: - Remove kernel test robot tag since it's a new driver. Change since v2: - Rename device tree nodes. - Fix unusual indentation. Change since v1: - Fix build test warning reported by kernel test robot. - Rename proper name for dt-bindings document. Neal Liu (3): usb: gadget: add Aspeed ast2600 udc driver ARM: dts: aspeed: Add USB2.0 device controller node dt-bindings: usb: add documentation for aspeed udc .../bindings/usb/aspeed,ast2600-udc.yaml | 52 + MAINTAINERS | 7 + arch/arm/boot/dts/aspeed-g6.dtsi | 10 + drivers/usb/gadget/udc/Kconfig | 13 + drivers/usb/gadget/udc/Makefile | 1 + drivers/usb/gadget/udc/aspeed_udc.c | 1601 +++++++++++++++++ 6 files changed, 1684 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/aspeed,ast2600-udc.yaml create mode 100644 drivers/usb/gadget/udc/aspeed_udc.c -- 2.25.1

3 years, 1 month

[PATCH v3 0/3] add Aspeed udc driver for ast2600

by Neal Liu

This patch series aim to add Aspeed USB 2.0 Device Controller (udc) driver, including driver itself, device tree node and documentation. Change since v2: - Rename device tree nodes. - Fix unusual indentation. Change since v1: - Fix build test warning reported by kernel test robot. - Rename proper name for dt-bindings document. *** BLURB HERE *** Neal Liu (3): usb: gadget: add Aspeed ast2600 udc driver ARM: dts: aspeed: Add USB2.0 device controller node dt-bindings: usb: add documentation for aspeed udc .../bindings/usb/aspeed,ast2600-udc.yaml | 52 + MAINTAINERS | 7 + arch/arm/boot/dts/aspeed-g6.dtsi | 10 + drivers/usb/gadget/udc/Kconfig | 13 + drivers/usb/gadget/udc/Makefile | 1 + drivers/usb/gadget/udc/aspeed_udc.c | 1601 +++++++++++++++++ 6 files changed, 1684 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/aspeed,ast2600-udc.yaml create mode 100644 drivers/usb/gadget/udc/aspeed_udc.c -- 2.25.1

3 years, 1 month

[RFC PATCH] procfs: Add file path and size to /proc/<pid>/fdinfo

by Kalesh Singh

Processes can pin shared memory by keeping a handle to it through a file descriptor; for instance dmabufs, memfd, and ashsmem (in Android). In the case of a memory leak, to identify the process pinning the memory, userspace needs to: - Iterate the /proc/<pid>/fd/* for each process - Do a readlink on each entry to identify the type of memory from the file path. - stat() each entry to get the size of the memory. The file permissions on /proc/<pid>/fd/* only allows for the owner or root to perform the operations above; and so is not suitable for capturing the system-wide state in a production environment. This issue was addressed for dmabufs by making /proc/*/fdinfo/* accessible to a process with PTRACE_MODE_READ_FSCREDS credentials[1] To allow the same kind of tracking for other types of shared memory, add the following fields to /proc/<pid>/fdinfo/<fd>: path - This allows identifying the type of memory based on common prefixes: e.g. "/memfd...", "/dmabuf...", "/dev/ashmem..." This was not an issued when dmabuf tracking was introduced because the exp_name field of dmabuf fdinfo could be used to distinguish dmabuf fds from other types. size - To track the amount of memory that is being pinned. dmabufs expose size as an additional field in fdinfo. Remove this and make it a common field for all fds. Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS -- the same as for /proc/<pid>/maps which also exposes the path and size for mapped memory regions. This allows for a system process with PTRACE_MODE_READ_FSCREDS to account the pinned per-process memory via fdinfo. [1] https://lore.kernel.org/lkml/20210308170651.919148-1-kaleshsingh@google.com/ Signed-off-by: Kalesh Singh <kaleshsingh(a)google.com> --- Documentation/filesystems/proc.rst | 22 ++++++++++++++++++++-- drivers/dma-buf/dma-buf.c | 1 - fs/proc/fd.c | 9 +++++++-- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst index 061744c436d9..ad66d78aca51 100644 --- a/Documentation/filesystems/proc.rst +++ b/Documentation/filesystems/proc.rst @@ -1922,13 +1922,16 @@ if precise results are needed. 3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file --------------------------------------------------------------- This file provides information associated with an opened file. The regular -files have at least four fields -- 'pos', 'flags', 'mnt_id' and 'ino'. +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size', +and 'path'. + The 'pos' represents the current offset of the opened file in decimal form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the file has been created with [see open(2) for details] and 'mnt_id' represents mount ID of the file system containing the opened file [see 3.5 /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of -the file. +the file, 'size' represents the size of the file in bytes, and 'path' +represents the file path. A typical output is:: @@ -1936,6 +1939,8 @@ A typical output is:: flags: 0100002 mnt_id: 19 ino: 63107 + size: 0 + path: /dev/null All locks associated with a file descriptor are shown in its fdinfo too:: @@ -1953,6 +1958,8 @@ Eventfd files flags: 04002 mnt_id: 9 ino: 63107 + size: 0 + path: anon_inode:[eventfd] eventfd-count: 5a where 'eventfd-count' is hex value of a counter. @@ -1966,6 +1973,8 @@ Signalfd files flags: 04002 mnt_id: 9 ino: 63107 + size: 0 + path: anon_inode:[signalfd] sigmask: 0000000000000200 where 'sigmask' is hex value of the signal mask associated @@ -1980,6 +1989,8 @@ Epoll files flags: 02 mnt_id: 9 ino: 63107 + size: 0 + path: anon_inode:[eventpoll] tfd: 5 events: 1d data: ffffffffffffffff pos:0 ino:61af sdev:7 where 'tfd' is a target file descriptor number in decimal form, @@ -1998,6 +2009,8 @@ For inotify files the format is the following:: flags: 02000000 mnt_id: 9 ino: 63107 + size: 0 + path: anon_inode:inotify inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d where 'wd' is a watch descriptor in decimal form, i.e. a target file @@ -2021,6 +2034,8 @@ For fanotify files the format is:: flags: 02 mnt_id: 9 ino: 63107 + size: 0 + path: anon_inode:[fanotify] fanotify flags:10 event-flags:0 fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003 fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4 @@ -2046,6 +2061,8 @@ Timerfd files flags: 02 mnt_id: 9 ino: 63107 + size: 0 + path: anon_inode:[timerfd] clockid: 0 ticks: 0 settime flags: 01 @@ -2070,6 +2087,7 @@ DMA Buffer files mnt_id: 9 ino: 63107 size: 32768 + path: /dmabuf: count: 2 exp_name: system-heap diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index b1e25ae98302..d61183ff3c30 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -377,7 +377,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) { struct dma_buf *dmabuf = file->private_data; - seq_printf(m, "size:\t%zu\n", dmabuf->size); /* Don't count the temporary reference taken inside procfs seq_show */ seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1); seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name); diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 913bef0d2a36..a8a968bc58f0 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -54,10 +54,15 @@ static int seq_show(struct seq_file *m, void *v) if (ret) return ret; - seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\n", + seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\nsize:\t%zu\n", (long long)file->f_pos, f_flags, real_mount(file->f_path.mnt)->mnt_id, - file_inode(file)->i_ino); + file_inode(file)->i_ino, + file_inode(file)->i_size); + + seq_puts(m, "path:\t"); + seq_file_path(m, file, "\n"); + seq_putc(m, '\n'); /* show_fd_locks() never deferences files so a stale value is safe */ show_fd_locks(m, file, files); base-commit: b015dcd62b86d298829990f8261d5d154b8d7af5 -- 2.36.1.124.g0e6072fb45-goog

3 years, 1 month

Re: [syzbot] WARNING in __dma_map_sg_attrs

by Daniel Vetter

On Sat, Feb 05, 2022 at 12:18:23PM -0800, syzbot wrote: > syzbot has found a reproducer for the following issue on: > > HEAD commit: 0457e5153e0e Merge tag 'for-linus' of git://git.kernel.org.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=11b2637c700000 > kernel config: https://syzkaller.appspot.com/x/.config?x=6f043113811433a5 > dashboard link: https://syzkaller.appspot.com/bug?extid=10e27961f4da37c443b2 > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11c65542700000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1163f480700000 > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+10e27961f4da37c443b2(a)syzkaller.appspotmail.com Adding Gerd, since this seems to blow up in udmabuf. I wonder why syzbot didn't figure this out, since it seems to have correctly added both dma-api and dma-buf people. Just not the maintainer for the begin_cpu_udmabuf function in the middle of the backtrace? -Daniel > > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 3595 at kernel/dma/mapping.c:188 __dma_map_sg_attrs+0x181/0x1f0 kernel/dma/mapping.c:188 > Modules linked in: > CPU: 0 PID: 3595 Comm: syz-executor249 Not tainted 5.17.0-rc2-syzkaller-00316-g0457e5153e0e #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > RIP: 0010:__dma_map_sg_attrs+0x181/0x1f0 kernel/dma/mapping.c:188 > Code: 00 00 00 00 00 fc ff df 48 c1 e8 03 80 3c 10 00 75 71 4c 8b 3d c0 83 b5 0d e9 db fe ff ff e8 b6 0f 13 00 0f 0b e8 af 0f 13 00 <0f> 0b 45 31 e4 e9 54 ff ff ff e8 a0 0f 13 00 49 8d 7f 50 48 b8 00 > RSP: 0018:ffffc90002a07d68 EFLAGS: 00010293 > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > RDX: ffff88807e25e2c0 RSI: ffffffff81649e91 RDI: ffff88801b848408 > RBP: ffff88801b848000 R08: 0000000000000002 R09: ffff88801d86c74f > R10: ffffffff81649d72 R11: 0000000000000001 R12: 0000000000000002 > R13: ffff88801d86c680 R14: 0000000000000001 R15: 0000000000000000 > FS: 0000555556e30300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000200000cc CR3: 000000001d74a000 CR4: 00000000003506e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <TASK> > dma_map_sgtable+0x70/0xf0 kernel/dma/mapping.c:264 > get_sg_table.isra.0+0xe0/0x160 drivers/dma-buf/udmabuf.c:72 > begin_cpu_udmabuf+0x130/0x1d0 drivers/dma-buf/udmabuf.c:126 > dma_buf_begin_cpu_access+0xfd/0x1d0 drivers/dma-buf/dma-buf.c:1164 > dma_buf_ioctl+0x259/0x2b0 drivers/dma-buf/dma-buf.c:363 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:874 [inline] > __se_sys_ioctl fs/ioctl.c:860 [inline] > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:860 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > RIP: 0033:0x7f62fcf530f9 > Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48 > RSP: 002b:00007ffe3edab9b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f62fcf530f9 > RDX: 0000000020000200 RSI: 0000000040086200 RDI: 0000000000000006 > RBP: 00007f62fcf170e0 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 00007f62fcf17170 > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 > </TASK> > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 1 month

[PATCH v2 0/3] add Aspeed udc driver for ast2600

by Neal Liu

This patch series aim to add Aspeed USB 2.0 Device Controller (udc) driver, including driver itself, device tree node and documentation. Change since v1: - Fix build test warning reported by kernel test robot. - Rename proper name for dt-bindings document. *** BLURB HERE *** Neal Liu (3): usb: gadget: add Aspeed ast2600 udc driver ARM: dts: aspeed: Add USB2.0 device controller node dt-bindings: usb: add documentation for aspeed udc .../bindings/usb/aspeed,ast2600-udc.yaml | 52 + MAINTAINERS | 7 + arch/arm/boot/dts/aspeed-g6.dtsi | 10 + drivers/usb/gadget/udc/Kconfig | 13 + drivers/usb/gadget/udc/Makefile | 1 + drivers/usb/gadget/udc/aspeed_udc.c | 1601 +++++++++++++++++ 6 files changed, 1684 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/aspeed,ast2600-udc.yaml create mode 100644 drivers/usb/gadget/udc/aspeed_udc.c -- 2.25.1

3 years, 1 month

[PATCH 0/3] add Aspeed udc driver for ast2600

by Neal Liu

This patch series aim to add Aspeed USB 2.0 Device Controller (udc) driver, including driver itself, device tree node and documentation. *** BLURB HERE *** Neal Liu (3): usb: gadget: add Aspeed ast2600 udc driver ARM: dts: aspeed: Add USB2.0 device controller node dt-bindings: usb: add documentation for aspeed udc .../devicetree/bindings/usb/aspeed,udc.yaml | 52 + MAINTAINERS | 7 + arch/arm/boot/dts/aspeed-g6.dtsi | 10 + drivers/usb/gadget/udc/Kconfig | 13 + drivers/usb/gadget/udc/Makefile | 1 + drivers/usb/gadget/udc/aspeed_udc.c | 1594 +++++++++++++++++ 6 files changed, 1677 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/aspeed,udc.yaml create mode 100644 drivers/usb/gadget/udc/aspeed_udc.c -- 2.25.1

3 years, 1 month

[PATCH V3 RESEND] dma-buf: ensure unique directory name for dmabuf stats

by Charan Teja Kalla

The dmabuf file uses get_next_ino()(through dma_buf_getfile() -> alloc_anon_inode()) to get an inode number and uses the same as a directory name under /sys/kernel/dmabuf/buffers/<ino>. This directory is used to collect the dmabuf stats and it is created through dma_buf_stats_setup(). At current, failure to create this directory entry can make the dma_buf_export() to fail. Now, as the get_next_ino() can definitely give a repetitive inode no causing the directory entry creation to fail with -EEXIST. This is a problem on the systems where dmabuf stats functionality is enabled on the production builds can make the dma_buf_export(), though the dmabuf memory is allocated successfully, to fail just because it couldn't create stats entry. This issue we are able to see on the snapdragon system within 13 days where there already exists a directory with inode no "122602" so dma_buf_stats_setup() failed with -EEXIST as it is trying to create the same directory entry. To make the dentry name as unique, use the dmabuf fs specific inode which is based on the simple atomic variable increment. There is tmpfs subsystem too which relies on its own inode generation rather than relying on the get_next_ino() for the same reason of avoiding the duplicate inodes[1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/?id=… Signed-off-by: Charan Teja Kalla <quic_charante(a)quicinc.com> Cc: <stable(a)vger.kernel.org> # 5.15.x+ Reviewed-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- Changes in V3-resend: -- Collect all the tags and apply stable tag. Changes in V3: -- Used the atomic64 variable to have dmabuf files its own inodes. -- Ensured no UAPI breakage as suggested by Christian. Changes in V2: -- Used the atomic64_t variable to generate a unique_id to be appended to inode to have an unique directory with name <inode_number-unique_id> -- Suggested by christian -- Updated the ABI documentation -- Identified by Greg. -- Massaged the commit log. -- https://lore.kernel.org/all/1652191562-18700-1-git-send-email-quic_charante… Changes in V1: -- Used the inode->i_ctime->tv_secs as an id appended to inode to create the unique directory with name <inode_number-time_in_secs>. -- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante… drivers/dma-buf/dma-buf.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6fc96e..0ad5039 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -407,6 +407,7 @@ static inline int is_dma_buf_file(struct file *file) static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) { + static atomic64_t dmabuf_inode = ATOMIC64_INIT(0); struct file *file; struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb); @@ -416,6 +417,13 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) inode->i_size = dmabuf->size; inode_set_bytes(inode, dmabuf->size); + /* + * The ->i_ino acquired from get_next_ino() is not unique thus + * not suitable for using it as dentry name by dmabuf stats. + * Override ->i_ino with the unique and dmabuffs specific + * value. + */ + inode->i_ino = atomic64_add_return(1, &dmabuf_inode); file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf", flags, &dma_buf_fops); if (IS_ERR(file)) -- 2.7.4

3 years, 1 month

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig