All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: "Christian König" <christian.koenig@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Hridya Valsaraju <hridya@google.com>,
	Sumit Semwal <sumit.semwal@linaro.org>,
	linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org,
	john.stultz@linaro.org, surenb@google.com,
	kaleshsingh@google.com, tjmercier@google.com,
	keescook@google.com
Subject: Re: [PATCH] dma-buf: Move sysfs work out of DMA-BUF export/release path
Date: Tue, 11 Jan 2022 12:16:54 +0100	[thread overview]
Message-ID: <Yd1nJqmHXULnccNF@kroah.com> (raw)
In-Reply-To: <934ac18c-d53e-beeb-48c1-015a5936e713@amd.com>

On Tue, Jan 11, 2022 at 11:58:07AM +0100, Christian König wrote:
> > > This is also not a problem due to the high number of DMA-BUF
> > > exports during launch time, as even a single export can be delayed for
> > > an unpredictable amount of time. We cannot eliminate DMA-BUF exports
> > > completely during app-launches and we are unfortunately seeing reports
> > > of the exporting process occasionally sleeping long enough to cause
> > > user-visible jankiness :(
> > > 
> > > We also looked at whether any optimizations are possible from the
> > > kernfs implementation side[1] but the semaphore is used quite extensively
> > > and it looks like the best way forward would be to remove sysfs
> > > creation/teardown from the DMA-BUF export/release path altogether. We
> > > have some ideas on how we can reduce the code-complexity in the
> > > current patch. If we manage to
> > > simplify it considerably, would the approach of offloading sysfs
> > > creation and teardown into a separate thread be acceptable Christian?
> 
> At bare minimum I suggest to use a work_struct instead of re-inventing that
> with kthread.
> 
> And then only put the exporting of buffers into the background and not the
> teardown.
> 
> > > Thank you for the guidance!
> > One worry I have here with doing this async that now userspace might
> > have a dma-buf, but the sysfs entry does not yet exist, or the dma-buf
> > is gone, but the sysfs entry still exists. That's a bit awkward wrt
> > semantics.
> > 
> > Also I'm pretty sure that if we can hit this, then other subsystems
> > using kernfs have similar problems, so trying to fix this in kernfs
> > with slightly more fine-grained locking sounds like a much more solid
> > approach. The linked patch talks about how the big delays happen due
> > to direct reclaim, and that might be limited to specific code paths
> > that we need to look at? As-is this feels a bit much like papering
> > over kernfs issues in hackish ways in sysfs users, instead of tackling
> > the problem at its root.
> 
> Which is exactly my feeling as well, yes.

More and more people are using sysfs/kernfs now for things that it was
never designed for (i.e. high-speed statistic gathering).  That's not
the fault of kernfs, it's the fault of people thinking it can be used
for stuff like that :)

But delays like this is odd, tearing down sysfs attributes should
normally _never_ be a fast-path that matters to system throughput.  So
offloading it to a workqueue makes sense as the attributes here are for
objects that are on the fast-path.

thanks,

greg k-h

WARNING: multiple messages have this Message-ID (diff)
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: "Christian König" <christian.koenig@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	keescook@google.com, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, tjmercier@google.com,
	linaro-mm-sig@lists.linaro.org, kaleshsingh@google.com,
	Hridya Valsaraju <hridya@google.com>,
	surenb@google.com, linux-media@vger.kernel.org
Subject: Re: [PATCH] dma-buf: Move sysfs work out of DMA-BUF export/release path
Date: Tue, 11 Jan 2022 12:16:54 +0100	[thread overview]
Message-ID: <Yd1nJqmHXULnccNF@kroah.com> (raw)
In-Reply-To: <934ac18c-d53e-beeb-48c1-015a5936e713@amd.com>

On Tue, Jan 11, 2022 at 11:58:07AM +0100, Christian König wrote:
> > > This is also not a problem due to the high number of DMA-BUF
> > > exports during launch time, as even a single export can be delayed for
> > > an unpredictable amount of time. We cannot eliminate DMA-BUF exports
> > > completely during app-launches and we are unfortunately seeing reports
> > > of the exporting process occasionally sleeping long enough to cause
> > > user-visible jankiness :(
> > > 
> > > We also looked at whether any optimizations are possible from the
> > > kernfs implementation side[1] but the semaphore is used quite extensively
> > > and it looks like the best way forward would be to remove sysfs
> > > creation/teardown from the DMA-BUF export/release path altogether. We
> > > have some ideas on how we can reduce the code-complexity in the
> > > current patch. If we manage to
> > > simplify it considerably, would the approach of offloading sysfs
> > > creation and teardown into a separate thread be acceptable Christian?
> 
> At bare minimum I suggest to use a work_struct instead of re-inventing that
> with kthread.
> 
> And then only put the exporting of buffers into the background and not the
> teardown.
> 
> > > Thank you for the guidance!
> > One worry I have here with doing this async that now userspace might
> > have a dma-buf, but the sysfs entry does not yet exist, or the dma-buf
> > is gone, but the sysfs entry still exists. That's a bit awkward wrt
> > semantics.
> > 
> > Also I'm pretty sure that if we can hit this, then other subsystems
> > using kernfs have similar problems, so trying to fix this in kernfs
> > with slightly more fine-grained locking sounds like a much more solid
> > approach. The linked patch talks about how the big delays happen due
> > to direct reclaim, and that might be limited to specific code paths
> > that we need to look at? As-is this feels a bit much like papering
> > over kernfs issues in hackish ways in sysfs users, instead of tackling
> > the problem at its root.
> 
> Which is exactly my feeling as well, yes.

More and more people are using sysfs/kernfs now for things that it was
never designed for (i.e. high-speed statistic gathering).  That's not
the fault of kernfs, it's the fault of people thinking it can be used
for stuff like that :)

But delays like this is odd, tearing down sysfs attributes should
normally _never_ be a fast-path that matters to system throughput.  So
offloading it to a workqueue makes sense as the attributes here are for
objects that are on the fast-path.

thanks,

greg k-h

  reply	other threads:[~2022-01-11 11:16 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-04 23:51 [PATCH] dma-buf: Move sysfs work out of DMA-BUF export/release path Hridya Valsaraju
2022-01-04 23:51 ` Hridya Valsaraju
2022-01-05 15:13 ` Greg Kroah-Hartman
2022-01-05 15:13   ` Greg Kroah-Hartman
2022-01-05 18:38   ` Hridya Valsaraju
2022-01-05 18:38     ` Hridya Valsaraju
2022-01-06  8:59 ` Christian König
2022-01-06  8:59   ` Christian König
2022-01-06 19:04   ` Hridya Valsaraju
2022-01-06 19:04     ` Hridya Valsaraju
2022-01-07 10:22     ` Christian König
2022-01-07 10:22       ` Christian König
2022-01-07 18:17       ` Hridya Valsaraju
2022-01-07 18:17         ` Hridya Valsaraju
2022-01-07 21:25         ` Hridya Valsaraju
2022-01-07 21:25           ` Hridya Valsaraju
2022-01-10  7:28           ` Christian König
2022-01-10  7:28             ` Christian König
2022-01-11  6:01             ` Hridya Valsaraju
2022-01-11  6:01               ` Hridya Valsaraju
2022-01-11  8:35               ` Daniel Vetter
2022-01-11  8:35                 ` Daniel Vetter
2022-01-11 10:58                 ` Christian König
2022-01-11 10:58                   ` Christian König
2022-01-11 11:16                   ` Greg Kroah-Hartman [this message]
2022-01-11 11:16                     ` Greg Kroah-Hartman
2022-01-11 11:43                     ` Christian König
2022-01-11 11:43                       ` Christian König
2022-01-12  1:11                       ` Hridya Valsaraju
2022-01-12  1:11                         ` Hridya Valsaraju

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Yd1nJqmHXULnccNF@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=christian.koenig@amd.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hridya@google.com \
    --cc=john.stultz@linaro.org \
    --cc=kaleshsingh@google.com \
    --cc=keescook@google.com \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=sumit.semwal@linaro.org \
    --cc=surenb@google.com \
    --cc=tjmercier@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.