linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
@ 2017-02-22 17:41 Harald Geyer
  2017-02-22 17:41 ` [PATCH 2/2] regulator: core: Fix race on multiple calls to regulator_disable_deferred() Harald Geyer
  2017-02-22 18:21 ` [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown
  0 siblings, 2 replies; 14+ messages in thread
From: Harald Geyer @ 2017-02-22 17:41 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Tejun Heo, Lai Jiangshan
  Cc: linux-kernel, Harald Geyer

Drivers calling queue_delayed_work() or mod_delayed_work() multiple times
on the same work without coordination get undefined behaviour. Add a new
function, which is easier to use.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 include/linux/workqueue.h | 17 +++++++++++++++++
 kernel/workqueue.c        | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index fc6e221..d79421c 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -433,6 +433,8 @@ extern bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 			struct delayed_work *work, unsigned long delay);
 extern bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
 			struct delayed_work *dwork, unsigned long delay);
+extern bool mod_fwd_delayed_work_on(int cpu, struct workqueue_struct *wq,
+			struct delayed_work *dwork, unsigned long delay);
 
 extern void flush_workqueue(struct workqueue_struct *wq);
 extern void drain_workqueue(struct workqueue_struct *wq);
@@ -505,6 +507,21 @@ static inline bool mod_delayed_work(struct workqueue_struct *wq,
 }
 
 /**
+ * mod_fwd_delayed_work - queue a delayed work or increase delay
+ * @wq: workqueue to use
+ * @dwork: work to queue
+ * @delay: number of jiffies to wait before queueing
+ *
+ * mod_fwd_delayed_work_on() on local CPU.
+ */
+static inline bool mod_fwd_delayed_work(struct workqueue_struct *wq,
+					struct delayed_work *dwork,
+					unsigned long delay)
+{
+	return mod_fwd_delayed_work_on(WORK_CPU_UNBOUND, wq, dwork, delay);
+}
+
+/**
  * schedule_work_on - put work task on a specific cpu
  * @cpu: cpu to put the work task on
  * @work: job to be done
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 479d840..30837e6 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1603,6 +1603,47 @@ bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
 EXPORT_SYMBOL_GPL(mod_delayed_work_on);
 
 /**
+ * mod_fwd_delayed_work_on - like mod_delayed_work(), but only increase delay
+ * @cpu: CPU number to execute work on
+ * @wq: workqueue to use
+ * @dwork: work to queue
+ * @delay: number of jiffies to wait before queueing
+ *
+ * If @dwork is idle, equivalent to queue_delayed_work_on(); otherwise,
+ * compare the old expiration time with @delay and set @dwork's timer
+ * so that it expires after the later time.
+ *
+ * Return: %false if @dwork was idle and queued, %true if @dwork was
+ * pending and its timer was modified.
+ *
+ * This function is safe to call from any context including IRQ handler.
+ * See try_to_grab_pending() for details.
+ */
+bool mod_fwd_delayed_work_on(int cpu, struct workqueue_struct *wq,
+			     struct delayed_work *dwork, unsigned long delay)
+{
+	unsigned long flags;
+	int ret;
+
+	do {
+		ret = try_to_grab_pending(&dwork->work, true, &flags);
+	} while (unlikely(ret == -EAGAIN));
+
+	if (unlikely(ret == 1 &&
+		     time_after(dwork->timer.expires, jiffies + delay)))
+		delay = dwork->timer.expires - jiffies;
+
+	if (likely(ret >= 0)) {
+		__queue_delayed_work(cpu, wq, dwork, delay);
+		local_irq_restore(flags);
+	}
+
+	/* -ENOENT from try_to_grab_pending() becomes %true */
+	return ret;
+}
+EXPORT_SYMBOL_GPL(mod_fwd_delayed_work_on);
+
+/**
  * worker_enter_idle - enter idle state
  * @worker: worker which is entering idle state
  *
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] regulator: core: Fix race on multiple calls to regulator_disable_deferred()
  2017-02-22 17:41 [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Harald Geyer
@ 2017-02-22 17:41 ` Harald Geyer
  2017-02-22 18:21 ` [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown
  1 sibling, 0 replies; 14+ messages in thread
From: Harald Geyer @ 2017-02-22 17:41 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Tejun Heo, Lai Jiangshan
  Cc: linux-kernel, Harald Geyer

The old code has two issues:
1) When multiple consumers call regulator_disable_deferred() close in time,
always the delay requested in the first call is used.
2) When a consumer calls regulator_disable_deferred(), but enables and
calls regulator_disable_deferred() again before the timer fires, the timer
isn't reset.

Both issues can cause the regulator to get disabled early.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/regulator/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cc68604..ce4923b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2449,8 +2449,8 @@ int regulator_disable_deferred(struct regulator *regulator, int ms)
 	rdev->deferred_disables++;
 	mutex_unlock(&rdev->mutex);
 
-	queue_delayed_work(system_power_efficient_wq, &rdev->disable_work,
-			   msecs_to_jiffies(ms));
+	mod_fwd_delayed_work(system_power_efficient_wq, &rdev->disable_work,
+			     msecs_to_jiffies(ms));
 	return 0;
 }
 EXPORT_SYMBOL_GPL(regulator_disable_deferred);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-22 17:41 [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Harald Geyer
  2017-02-22 17:41 ` [PATCH 2/2] regulator: core: Fix race on multiple calls to regulator_disable_deferred() Harald Geyer
@ 2017-02-22 18:21 ` Mark Brown
  2017-02-22 20:06   ` Harald Geyer
  1 sibling, 1 reply; 14+ messages in thread
From: Mark Brown @ 2017-02-22 18:21 UTC (permalink / raw)
  To: Harald Geyer; +Cc: Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Wed, Feb 22, 2017 at 05:41:24PM +0000, Harald Geyer wrote:
> Drivers calling queue_delayed_work() or mod_delayed_work() multiple times
> on the same work without coordination get undefined behaviour. Add a new
> function, which is easier to use.

The obvious question here, especially in the case of mod_delayed_work(),
is why not fix the existing functions to have the expected behaviour?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-22 18:21 ` [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown
@ 2017-02-22 20:06   ` Harald Geyer
  2017-02-23 17:34     ` Mark Brown
  0 siblings, 1 reply; 14+ messages in thread
From: Harald Geyer @ 2017-02-22 20:06 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

Mark Brown writes:
> On Wed, Feb 22, 2017 at 05:41:24PM +0000, Harald Geyer wrote:
> > Drivers calling queue_delayed_work() or mod_delayed_work() multiple times
> > on the same work without coordination get undefined behaviour. Add a new
> > function, which is easier to use.
> 
> The obvious question here, especially in the case of mod_delayed_work(),
> is why not fix the existing functions to have the expected behaviour?

AFAICS the existing functions behave as documented. I don't feel to be an
authority to decide that the documented behaviour is not right. Actually
I think that what mod_delayed_work() does, is a valid operation, even
if many current users probably don't want it.

I guess many users don't care, because they are calling mod_delayed_work()
from only a single place with a constant delay. However reviewing all
107 use cases in 58 files to check if we can safely change the
behaviour, would be quite a lot of work.

I was suprised when I found that no function like mod_fwd_delayed_work()
existed, so you have a point there.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-22 20:06   ` Harald Geyer
@ 2017-02-23 17:34     ` Mark Brown
  2017-02-23 23:22       ` Harald Geyer
  2017-03-06 22:22       ` Tejun Heo
  0 siblings, 2 replies; 14+ messages in thread
From: Mark Brown @ 2017-02-23 17:34 UTC (permalink / raw)
  To: Harald Geyer; +Cc: Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

On Wed, Feb 22, 2017 at 09:06:23PM +0100, Harald Geyer wrote:
> Mark Brown writes:
> > On Wed, Feb 22, 2017 at 05:41:24PM +0000, Harald Geyer wrote:
> > > Drivers calling queue_delayed_work() or mod_delayed_work() multiple times
> > > on the same work without coordination get undefined behaviour. Add a new
> > > function, which is easier to use.

> > The obvious question here, especially in the case of mod_delayed_work(),
> > is why not fix the existing functions to have the expected behaviour?

> AFAICS the existing functions behave as documented. I don't feel to be an
> authority to decide that the documented behaviour is not right. Actually
> I think that what mod_delayed_work() does, is a valid operation, even
> if many current users probably don't want it.

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.

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-23 17:34     ` Mark Brown
@ 2017-02-23 23:22       ` Harald Geyer
  2017-02-27 12:54         ` Mark Brown
  2017-03-06 22:22       ` Tejun Heo
  1 sibling, 1 reply; 14+ messages in thread
From: Harald Geyer @ 2017-02-23 23:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Harald Geyer, Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

Mark Brown writes:
> > > The obvious question here, especially in the case of
> > > mod_delayed_work(), is why not fix the existing functions to have
> > > the expected behaviour?
> 
> > AFAICS the existing functions behave as documented. I don't feel
> > to be an authority to decide that the documented behaviour is not
> > right. Actually I think that what mod_delayed_work() does, is a valid
> > operation, even if many current users probably don't want it.
> 
> 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.

It was obvious enough for me, so that I proposed a new function
instead of just switching the regulator code from queue_delayed_work()
to mod_delayed_work(). If it's not obvious to you, I suggest that
you supply a patch improving the documentation.

> > 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.

As I already wrote in my last message: To go that path means to review
107 uses of mod_delayed_work(). Maybe you have somebody you can assign
that task to? 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-23 23:22       ` Harald Geyer
@ 2017-02-27 12:54         ` Mark Brown
  2017-02-27 19:17           ` Harald Geyer
  0 siblings, 1 reply; 14+ messages in thread
From: Mark Brown @ 2017-02-27 12:54 UTC (permalink / raw)
  To: Harald Geyer; +Cc: Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Fri, Feb 24, 2017 at 12:22:37AM +0100, Harald Geyer wrote:
> Mark Brown writes:

> > 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.

> It was obvious enough for me, so that I proposed a new function
> instead of just switching the regulator code from queue_delayed_work()
> to mod_delayed_work(). If it's not obvious to you, I suggest that
> you supply a patch improving the documentation.

I'd need to figure out exactly what the restrictions are and like I say
the name of the function itself is confusing, I suspect because it
predates SMP.

> > 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.

> As I already wrote in my last message: To go that path means to review
> 107 uses of mod_delayed_work(). Maybe you have somebody you can assign
> that task to? 

Actually yes, though not immediately.  Another option is to just rename
the current function and all the callers en masse then add a new, safe
mod_delayed_work().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-27 12:54         ` Mark Brown
@ 2017-02-27 19:17           ` Harald Geyer
  2017-03-07 12:42             ` Mark Brown
  0 siblings, 1 reply; 14+ messages in thread
From: Harald Geyer @ 2017-02-27 19:17 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

Mark Brown writes:
> On Fri, Feb 24, 2017 at 12:22:37AM +0100, Harald Geyer wrote:
> > Mark Brown writes:
> 
> > > 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.
> 
> > It was obvious enough for me, so that I proposed a new function
> > instead of just switching the regulator code from queue_delayed_work()
> > to mod_delayed_work(). If it's not obvious to you, I suggest that
> > you supply a patch improving the documentation.
> 
> I'd need to figure out exactly what the restrictions are and like I say
> the name of the function itself is confusing, I suspect because it
> predates SMP.

I guess you know that, but just to avoid any confusion: The bug in the
regulator code is not related to SMP at all.
 
> > > 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.
> 
> > As I already wrote in my last message: To go that path means to review
> > 107 uses of mod_delayed_work(). Maybe you have somebody you can assign
> > that task to? 
> 
> Actually yes, though not immediately.  Another option is to just rename
> the current function and all the callers en masse then add a new, safe
> mod_delayed_work().

Okay by me. I'm removing the issue from my todo list and hand it over
to you and your minions ... :)

thanks,
Harald

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-23 17:34     ` Mark Brown
  2017-02-23 23:22       ` Harald Geyer
@ 2017-03-06 22:22       ` Tejun Heo
  2017-03-07 11:29         ` Harald Geyer
  2017-03-07 12:45         ` Mark Brown
  1 sibling, 2 replies; 14+ messages in thread
From: Tejun Heo @ 2017-03-06 22:22 UTC (permalink / raw)
  To: Mark Brown; +Cc: Harald Geyer, Liam Girdwood, Lai Jiangshan, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-03-06 22:22       ` Tejun Heo
@ 2017-03-07 11:29         ` Harald Geyer
  2017-03-07 19:16           ` Tejun Heo
  2017-03-07 12:45         ` Mark Brown
  1 sibling, 1 reply; 14+ messages in thread
From: Harald Geyer @ 2017-03-07 11:29 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Mark Brown, Liam Girdwood, Lai Jiangshan, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-02-27 19:17           ` Harald Geyer
@ 2017-03-07 12:42             ` Mark Brown
  2017-03-10 18:44               ` Harald Geyer
  0 siblings, 1 reply; 14+ messages in thread
From: Mark Brown @ 2017-03-07 12:42 UTC (permalink / raw)
  To: Harald Geyer; +Cc: Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Mon, Feb 27, 2017 at 08:17:10PM +0100, Harald Geyer wrote:
> Mark Brown writes:

> > I'd need to figure out exactly what the restrictions are and like I say
> > the name of the function itself is confusing, I suspect because it
> > predates SMP.

> I guess you know that, but just to avoid any confusion: The bug in the
> regulator code is not related to SMP at all.

It's not?  Oh.  I had formed the impression that this was a race
condition.

> > Actually yes, though not immediately.  Another option is to just rename
> > the current function and all the callers en masse then add a new, safe
> > mod_delayed_work().

> Okay by me. I'm removing the issue from my todo list and hand it over
> to you and your minions ... :)

Well, we need to figure out what the desired solution is!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-03-06 22:22       ` Tejun Heo
  2017-03-07 11:29         ` Harald Geyer
@ 2017-03-07 12:45         ` Mark Brown
  1 sibling, 0 replies; 14+ messages in thread
From: Mark Brown @ 2017-03-07 12:45 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Harald Geyer, Liam Girdwood, Lai Jiangshan, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-03-07 11:29         ` Harald Geyer
@ 2017-03-07 19:16           ` Tejun Heo
  0 siblings, 0 replies; 14+ messages in thread
From: Tejun Heo @ 2017-03-07 19:16 UTC (permalink / raw)
  To: Harald Geyer; +Cc: Mark Brown, Liam Girdwood, Lai Jiangshan, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work()
  2017-03-07 12:42             ` Mark Brown
@ 2017-03-10 18:44               ` Harald Geyer
  0 siblings, 0 replies; 14+ messages in thread
From: Harald Geyer @ 2017-03-10 18:44 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, Tejun Heo, Lai Jiangshan, linux-kernel

Mark Brown writes:
> On Mon, Feb 27, 2017 at 08:17:10PM +0100, Harald Geyer wrote:
> > Mark Brown writes:
> 
> > > I'd need to figure out exactly what the restrictions are and like I say
> > > the name of the function itself is confusing, I suspect because it
> > > predates SMP.
> 
> > I guess you know that, but just to avoid any confusion: The bug in the
> > regulator code is not related to SMP at all.
> 
> It's not?  Oh.  I had formed the impression that this was a race
> condition.

I have used the word "race" in the description of patch 2/2 - sorry if
that was the wrong word, I'm trained in physics not computer science.
The description of patch 2/2 was:

> regulator: core: Fix race on multiple calls to regulator_disable_deferred()
>
> The old code has two issues:
> 1) When multiple consumers call regulator_disable_deferred() close in time,
> always the delay requested in the first call is used.
> 2) When a consumer calls regulator_disable_deferred(), but enables and
> calls regulator_disable_deferred() again before the timer fires, the timer
> isn't reset.
>
> Both issues can cause the regulator to get disabled early.

Issue (1) is what I had thought of a race - not in multiprocessing but
in multitasking. Please educate me, if this is not the case or if you can
think of a way how I could have expressed the idea more clearly.

> Well, we need to figure out what the desired solution is!

Do we have reached consensus on what to do? Do people wait for me to do some
work? Based on the comments in this thread it seems one way forward is,
that I'm resending the series with mod_fwd_delayed_work() renamed to
postpone_delayed_work(). Please let me know if you still consider
alternative approaches or need more time to evaluate the situation.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-03-10 18:44 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-22 17:41 [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Harald Geyer
2017-02-22 17:41 ` [PATCH 2/2] regulator: core: Fix race on multiple calls to regulator_disable_deferred() Harald Geyer
2017-02-22 18:21 ` [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown
2017-02-22 20:06   ` Harald Geyer
2017-02-23 17:34     ` Mark Brown
2017-02-23 23:22       ` Harald Geyer
2017-02-27 12:54         ` Mark Brown
2017-02-27 19:17           ` Harald Geyer
2017-03-07 12:42             ` Mark Brown
2017-03-10 18:44               ` Harald Geyer
2017-03-06 22:22       ` Tejun Heo
2017-03-07 11:29         ` Harald Geyer
2017-03-07 19:16           ` Tejun Heo
2017-03-07 12:45         ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).