linux-kernel - Re: [RFC v4 02/12] kthread: Add kthread_(un)block_work_queuing() and kthread_work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200511150251.GE16815@mtj.duckdns.org>
Date:   Mon, 11 May 2020 11:02:51 -0400
From:   Tejun Heo <tj@...nel.org>
To:     Lyude Paul <lyude@...hat.com>
Cc:     nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
        Ville Syrjälä 
        <ville.syrjala@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Petr Mladek <pmladek@...e.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Liang Chen <cl@...k-chips.com>,
        Ben Dooks <ben.dooks@...ethink.co.uk>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC v4 02/12] kthread: Add kthread_(un)block_work_queuing() and
 kthread_work_queuable()

On Fri, May 08, 2020 at 04:46:52PM -0400, Lyude Paul wrote:
> Add some simple wrappers around incrementing/decrementing
> kthread_work.cancelling under lock, along with checking whether queuing
> is currently allowed on a given kthread_work, which we'll use want to
> implement work cancelling with DRM's vblank work helpers.

Am I correct in assuming that what you want is "cancel this and block
further queueing until the state is cleared"? I agree that'd be something
really useful to have. That said, There are a few areas that I think can be
improved upon:

* It'd be better if we separate this state into its own thing rather than
  mixing with canceling state which has potential to make things really
  confusing. It doesn't have to be a separate field unless you want disable
  depth for work item disable (and I don't think you do). It can just be a
  high bit in the same field but I think the two states should be separate
  one way or the other.

* I'm always a bit skeptical about state querying interfaces which aren't
  synchronized to anything. They're useful in many cases but also prone to
  being misused. If you absoultely have to have them, can you please add
  explicit comment explaining the lack of synchronization around it - ie.
  unless you're the one setting and clearing the flag and queueing the task,
  it isn't synchronized against anything.

* In the same vein, I'm not too sure about stand-alone block interface.
  Unless I'm the sole queuer or there are further locking around queueing,
  what good does setting blocking do? There's no way to guarantee that the
  flag is seen by someone else trying to queue it and trying to flush the
  work item after queueing doesn't help either. The only way to make that
  interface meaningful is doing it together with cancel - so, you say "block
  further queueing and cancel / flush whatever is in flight or queued",
  which actually gives you a useful invariant.

* A simliar argument can be made about unblock too although that's an a lot
  more relaxed situation in that unblocking and queueing oneself always
  works and that the user might not care which future instance of queueing
  will start succeeding.

Thanks.

-- 
tejun