public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Lyude Paul <lyude@redhat.com>
Cc: nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, "Daniel Vetter" <daniel@ffwll.ch>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Suren Baghdasaryan" <surenb@google.com>,
	"Petr Mladek" <pmladek@suse.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Steven Rostedt (VMware)" <rostedt@goodmis.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ben Dooks" <ben.dooks@codethink.co.uk>,
	"Liang Chen" <cl@rock-chips.com>
Subject: Re: [RFC v4 01/12] kthread: Add kthread_queue_flush_work()
Date: Mon, 11 May 2020 10:49:35 -0400	[thread overview]
Message-ID: <20200511144935.GD16815@mtj.duckdns.org> (raw)
In-Reply-To: <20200508204751.155488-2-lyude@redhat.com>

Hello,

On Fri, May 08, 2020 at 04:46:51PM -0400, Lyude Paul wrote:
> +bool kthread_queue_flush_work(struct kthread_work *work,
> +			      struct kthread_flush_work *fwork);
> +void __kthread_flush_work_fn(struct kthread_work *work);

As an exposed interface, this doesn't seem great. What the user wants to say
is "wait for the current instance of this guy" and the interface is asking
them to queue an extra work item whose queueing return state should be
checked and depending on that result wait on its internal completion.

I'm skeptical this is a good idea in general given that unless you define
"this instance" at the time of queueing the work item which is being
waited-upon, there's no way to guarantee that the instance you're queueing
the flush work item on is the instance you want unless the queuer is holding
external synchronization which prevents the instance from running. That's a
really confusing semantics to expose in the interface.

What the above means is that the ordering that you want is only defined
through your own locking and that maybe suggests that the sequencing should
be implemented on that side too. It may be a bit more code but a sequence
counter + wait queue might be the better solution here.

Thanks.

-- 
tejun

  reply	other threads:[~2020-05-11 14:49 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-08 20:46 [RFC v4 00/12] drm/nouveau: Introduce CRC support for gf119+ Lyude Paul
2020-05-08 20:46 ` [RFC v4 01/12] kthread: Add kthread_queue_flush_work() Lyude Paul
2020-05-11 14:49   ` Tejun Heo [this message]
2020-05-11 15:02     ` Daniel Vetter
2020-05-08 20:46 ` [RFC v4 02/12] kthread: Add kthread_(un)block_work_queuing() and kthread_work_queuable() Lyude Paul
2020-05-11 15:02   ` Tejun Heo
2020-05-08 20:46 ` [RFC v4 03/12] drm/vblank: Register drmm cleanup action once per drm_vblank_crtc Lyude Paul
2020-05-08 20:59   ` Daniel Vetter
2020-05-08 20:46 ` [RFC v4 04/12] drm/vblank: Add vblank works Lyude Paul
2020-05-08 20:46 ` [RFC v4 05/12] drm/nouveau/kms/nv50-: Unroll error cleanup in nv50_head_create() Lyude Paul
2020-05-08 20:46 ` [RFC v4 06/12] drm/nouveau/kms/nv140-: Don't modify depth in state during atomic commit Lyude Paul
2020-05-08 20:46 ` [RFC v4 07/12] drm/nouveau/kms/nv50-: Fix disabling dithering Lyude Paul
2020-05-08 20:46 ` [RFC v4 08/12] drm/nouveau/kms/nv50-: s/harm/armh/g Lyude Paul
2020-05-08 20:46 ` [RFC v4 09/12] drm/nouveau/kms/nv140-: Track wndw mappings in nv50_head_atom Lyude Paul
2020-05-08 20:47 ` [RFC v4 10/12] drm/nouveau/kms/nv50-: Expose nv50_outp_atom in disp.h Lyude Paul
2020-05-08 20:47 ` [RFC v4 11/12] drm/nouveau/kms/nv50-: Move hard-coded object handles into header Lyude Paul
2020-05-08 20:47 ` [RFC v4 12/12] drm/nouveau/kms/nvd9-: Add CRC support Lyude Paul

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=20200511144935.GD16815@mtj.duckdns.org \
    --to=tj@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=ben.dooks@codethink.co.uk \
    --cc=cl@rock-chips.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lyude@redhat.com \
    --cc=nouveau@lists.freedesktop.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=surenb@google.com \
    --cc=tglx@linutronix.de \
    --cc=ville.syrjala@linux.intel.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox