Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1593782 > unrolled thread

Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Tejun Heo <tj@kernel.org> - 2017-03-06 23:40 +0100
    Re: [PATCH 1/2] workqueue: Add new function  mod_fwd_delayed_work() Harald Geyer <harald@ccbib.org> - 2017-03-07 13:20 +0100
      Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Tejun Heo <tj@kernel.org> - 2017-03-07 20:30 +0100
    Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown <broonie@kernel.org> - 2017-03-07 13:50 +0100

#1593782 — Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()

Hello,

On Thu, Feb 23, 2017 at 09:34:49AM -0800, Mark Brown wrote:
> It is *very* non-obvious that mod_delayed_work() will have a problem
> from the documentation, there's "mod_delayed_work_on() on local CPU" as
> the body of the description but honestly I'm struggling to tell if
> that's even there intentionally or anything other than an implementation
> detail.  I'd expect to see some words describing the situations where it
> can be used or something, both the name and the lack of any information
> about issues suggest it's the default thing and will work safely.

What the mod function is implemneting is "whoever wins (runs the last)
determines the timeout".  It is a bit unwiedly because unlike the
timer, workqueue delay takes duration instead of the absoulte time
making coordinating from the user side trickier.

> > I was suprised when I found that no function like mod_fwd_delayed_work()
> > existed, so you have a point there.
> 
> I suspect people are just using mod_delayed_work(), not realising that
> there are restrictions.  I'm thinking that perhaps it should be fixed
> to be safe for calling from different contexts and a new function with
> the existing behaviour added, that seems less error prone.

I don't think it's a matter of "fixing" the existing
mod_delayed_work().  What the new function is implementing wouldn't
fit use cases where the timeout should only be shortened (IIRC,
writeback code does that).

I'm not against adding new interface to handle it better but I think
it makes more sense to add both directions.  How about adding
expedite_delayed_work_on() and postpone_delayed_work_on()?

Thanks.

-- 
tejun

[toc] | [next] | [standalone]

#1594163 — Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()

On 06.03.2017 23:22, Tejun Heo wrote:
> I don't think it's a matter of "fixing" the existing
> mod_delayed_work().  What the new function is implementing wouldn't
> fit use cases where the timeout should only be shortened (IIRC,
> writeback code does that).
>
> I'm not against adding new interface to handle it better but I think
> it makes more sense to add both directions.  How about adding
> expedite_delayed_work_on() and postpone_delayed_work_on()?

I think such a function should only be added if there is actually
code using it. So I'd wait for the survey of existing 
mod_delayed_work()
users Mark has promised to actually find some bugs that would be fixed
by the function before adding it.

The names you are proposing feel less clear to me then mod_fwd/mod_bwd,
but english is not my native tongue, so my feeling is probably no
strong evidence ... :)

Otherwise I agree with your comments.

Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w
or CLAM xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN

[toc] | [prev] | [next] | [standalone]

#1594568

Hello,

On Tue, Mar 07, 2017 at 12:29:32PM +0100, Harald Geyer wrote:
> On 06.03.2017 23:22, Tejun Heo wrote:
> > I don't think it's a matter of "fixing" the existing
> > mod_delayed_work().  What the new function is implementing wouldn't
> > fit use cases where the timeout should only be shortened (IIRC,
> > writeback code does that).
> > 
> > I'm not against adding new interface to handle it better but I think
> > it makes more sense to add both directions.  How about adding
> > expedite_delayed_work_on() and postpone_delayed_work_on()?
> 
> I think such a function should only be added if there is actually
> code using it. So I'd wait for the survey of existing mod_delayed_work()
> users Mark has promised to actually find some bugs that would be fixed
> by the function before adding it.

I clearly remember writing code to work around that.  Unfortunately, I
can't find it right now. :( Note that these cases may use cancel,
update timeout, requeue sequence rather then mod_delayed_work().  But
yeah, we can add the other direction when we find and can convert the
users.

> The names you are proposing feel less clear to me then mod_fwd/mod_bwd,
> but english is not my native tongue, so my feeling is probably no
> strong evidence ... :)

Forward and backward aren't necessarily time related.  It can be
interpreted as relative position from now too - pulling a work item
forward or pushing it backward.  The orientation isn't explicit in the
name.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1594193

[Multipart message — attachments visible in raw view] — view raw

On Mon, Mar 06, 2017 at 05:22:12PM -0500, Tejun Heo wrote:
> On Thu, Feb 23, 2017 at 09:34:49AM -0800, Mark Brown wrote:

> > It is *very* non-obvious that mod_delayed_work() will have a problem
> > from the documentation, there's "mod_delayed_work_on() on local CPU" as
> > the body of the description but honestly I'm struggling to tell if
> > that's even there intentionally or anything other than an implementation
> > detail.  I'd expect to see some words describing the situations where it
> > can be used or something, both the name and the lack of any information
> > about issues suggest it's the default thing and will work safely.

> What the mod function is implemneting is "whoever wins (runs the last)
> determines the timeout".  It is a bit unwiedly because unlike the
> timer, workqueue delay takes duration instead of the absoulte time
> making coordinating from the user side trickier.

Ah, OK.  That's what the regulator API was looking for (the timeout is
always the same, we just want to kick the starting point into the
future).  We're not trying to reduce timeouts, only increase them.

> > I suspect people are just using mod_delayed_work(), not realising that
> > there are restrictions.  I'm thinking that perhaps it should be fixed
> > to be safe for calling from different contexts and a new function with
> > the existing behaviour added, that seems less error prone.

> I don't think it's a matter of "fixing" the existing
> mod_delayed_work().  What the new function is implementing wouldn't
> fit use cases where the timeout should only be shortened (IIRC,
> writeback code does that).

> I'm not against adding new interface to handle it better but I think
> it makes more sense to add both directions.  How about adding
> expedite_delayed_work_on() and postpone_delayed_work_on()?

That works for me.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web