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

Groups > linux.kernel > #1586380 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Harald Geyer <harald@ccbib.org> - 2017-02-22 19:30 +0100
    Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown <broonie@kernel.org> - 2017-02-22 19:30 +0100
      Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Harald Geyer <harald@ccbib.org> - 2017-02-22 21:10 +0100
        Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown <broonie@kernel.org> - 2017-02-23 18:50 +0100
          Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Harald Geyer <harald@ccbib.org> - 2017-02-24 00:30 +0100
            Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Mark Brown <broonie@kernel.org> - 2017-02-27 14:00 +0100
              Re: [PATCH 1/2] workqueue: Add new function mod_fwd_delayed_work() Harald Geyer <harald@ccbib.org> - 2017-02-27 20:50 +0100

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

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

[toc] | [next] | [standalone]

#1586383

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

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?

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

#1586448

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

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

#1587049

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

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.

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

#1587170

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

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

#1588657

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

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

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

#1588895

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web