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

Groups > linux.kernel > #1649001 > unrolled thread

[PATCH] workqueue: Implement delayed_work_busy()

Back to article view | Back to linux.kernel

Contents

  [PATCH] workqueue: Implement delayed_work_busy() Alex Naidis <alex.naidis@linux.com> - 2017-05-24 01:40 +0200
    Re: [PATCH] workqueue: Implement delayed_work_busy() Tejun Heo <tj@kernel.org> - 2017-05-24 17:20 +0200
      Fwd: [PATCH] workqueue: Implement delayed_work_busy() Alex Naidis <alex.naidis@linux.com> - 2017-05-24 22:10 +0200
      Re: [PATCH] workqueue: Implement delayed_work_busy() Tejun Heo <tj@kernel.org> - 2017-05-24 22:10 +0200
        Re: [PATCH] workqueue: Implement delayed_work_busy() Alex Naidis <alex.naidis@linux.com> - 2017-05-24 22:20 +0200
          Re: [PATCH] workqueue: Implement delayed_work_busy() Tejun Heo <tj@kernel.org> - 2017-05-24 22:30 +0200
            Re: [PATCH] workqueue: Implement delayed_work_busy() Alex Naidis <alex.naidis@linux.com> - 2017-05-24 22:40 +0200

#1649001 — [PATCH] workqueue: Implement delayed_work_busy()

This implements a variant of work_busy() for
delayed work.

CC: linux-kernel@vger.kernel.org
Signed-off-by: Alex Naidis <alex.naidis@linux.com>
---
 include/linux/workqueue.h | 1 +
 kernel/workqueue.c        | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index c102ef6..32ab046 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -467,6 +467,7 @@ extern void workqueue_set_max_active(struct workqueue_struct *wq,
 extern bool current_is_workqueue_rescuer(void);
 extern bool workqueue_congested(int cpu, struct workqueue_struct *wq);
 extern unsigned int work_busy(struct work_struct *work);
+extern unsigned int delayed_work_busy(struct delayed_work *dwork);
 extern __printf(1, 2) void set_worker_desc(const char *fmt, ...);
 extern void print_worker_info(const char *log_lvl, struct task_struct *task);
 extern void show_workqueue_state(void);
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c74bf39..658cc2e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4224,6 +4224,15 @@ unsigned int work_busy(struct work_struct *work)
 }
 EXPORT_SYMBOL_GPL(work_busy);
 
+/*
+ * See work_busy()
+ */
+unsigned int delayed_work_busy(struct delayed_work *dwork)
+{
+	return work_busy(&dwork->work);
+}
+EXPORT_SYMBOL_GPL(delayed_work_busy);
+
 /**
  * set_worker_desc - set description for the current work item
  * @fmt: printf-style format string
-- 
2.7.4

[toc] | [next] | [standalone]

#1649704

Hello,

On Wed, May 24, 2017 at 01:34:53AM +0200, Alex Naidis wrote:
> This implements a variant of work_busy() for
> delayed work.
> 
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Alex Naidis <alex.naidis@linux.com>
> ---
>  include/linux/workqueue.h | 1 +
>  kernel/workqueue.c        | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index c102ef6..32ab046 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -467,6 +467,7 @@ extern void workqueue_set_max_active(struct workqueue_struct *wq,
>  extern bool current_is_workqueue_rescuer(void);
>  extern bool workqueue_congested(int cpu, struct workqueue_struct *wq);
>  extern unsigned int work_busy(struct work_struct *work);
> +extern unsigned int delayed_work_busy(struct delayed_work *dwork);
>  extern __printf(1, 2) void set_worker_desc(const char *fmt, ...);
>  extern void print_worker_info(const char *log_lvl, struct task_struct *task);
>  extern void show_workqueue_state(void);
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c74bf39..658cc2e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4224,6 +4224,15 @@ unsigned int work_busy(struct work_struct *work)
>  }
>  EXPORT_SYMBOL_GPL(work_busy);
>  
> +/*
> + * See work_busy()
> + */
> +unsigned int delayed_work_busy(struct delayed_work *dwork)
> +{
> +	return work_busy(&dwork->work);
> +}
> +EXPORT_SYMBOL_GPL(delayed_work_busy);

What's the use case?  Some of existing work_busy() seem wrong already.
Also, if we need this, let's make it an inline function.

Thanks.

-- 
tejun

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

#1649902

Hello,
thank you for your quick review!

2017-05-24 17:11 GMT+02:00 Tejun Heo <tj@kernel.org>:
>
> Hello,
>
> On Wed, May 24, 2017 at 01:34:53AM +0200, Alex Naidis wrote:
> > This implements a variant of work_busy() for
> > delayed work.
> >
> > CC: linux-kernel@vger.kernel.org
> > Signed-off-by: Alex Naidis <alex.naidis@linux.com>
> > ---
> >  include/linux/workqueue.h | 1 +
> >  kernel/workqueue.c        | 9 +++++++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > index c102ef6..32ab046 100644
> > --- a/include/linux/workqueue.h
> > +++ b/include/linux/workqueue.h
> > @@ -467,6 +467,7 @@ extern void workqueue_set_max_active(struct workqueue_struct *wq,
> >  extern bool current_is_workqueue_rescuer(void);
> >  extern bool workqueue_congested(int cpu, struct workqueue_struct *wq);
> >  extern unsigned int work_busy(struct work_struct *work);
> > +extern unsigned int delayed_work_busy(struct delayed_work *dwork);
> >  extern __printf(1, 2) void set_worker_desc(const char *fmt, ...);
> >  extern void print_worker_info(const char *log_lvl, struct task_struct *task);
> >  extern void show_workqueue_state(void);
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index c74bf39..658cc2e 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -4224,6 +4224,15 @@ unsigned int work_busy(struct work_struct *work)
> >  }
> >  EXPORT_SYMBOL_GPL(work_busy);
> >
> > +/*
> > + * See work_busy()
> > + */
> > +unsigned int delayed_work_busy(struct delayed_work *dwork)
> > +{
> > +     return work_busy(&dwork->work);
> > +}
> > +EXPORT_SYMBOL_GPL(delayed_work_busy);
>
> What's the use case?  Some of existing work_busy() seem wrong already.

Yeah, I agree, it is wrong to rely on work_busy() providing correct data.
However sometimes it is useful to have an indicator like this to at least
catch some cases where requeuing work would be obsolete.
This applies for delayed work too.

>
> Also, if we need this, let's make it an inline function.

Alright, you should receive V2 of this patch right after this mail.
>
>
> Thanks.
>
> --
> tejun


Thank you again!
Alex

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

#1649905

Hello, Alex.

On Wed, May 24, 2017 at 09:58:19PM +0200, Alex Naidis wrote:
> Yeah, I agree, it is wrong to rely on work_busy() providing correct data.
> However sometimes it is useful to have an indicator like this to at least
> catch some cases where requeuing work would be obsolete.
> This applies for delayed work too.

Can you elaborage on "requeueing work would be obsolate" a bit?

Thanks.

-- 
tejun

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

#1649922

Hello,

2017-05-24 22:01 GMT+02:00 Tejun Heo <tj@kernel.org>:
> Hello, Alex.
>
> On Wed, May 24, 2017 at 09:58:19PM +0200, Alex Naidis wrote:
>> Yeah, I agree, it is wrong to rely on work_busy() providing correct data.
>> However sometimes it is useful to have an indicator like this to at least
>> catch some cases where requeuing work would be obsolete.
>> This applies for delayed work too.
>
> Can you elaborage on "requeueing work would be obsolate" a bit?
Sure.
The case that I mean is represented well by the usage of work_busy here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/acpi/bus.c#n534
It checks work_busy() before requeuing the work, since in some cases
it is obsolete
to requeue the work if it is already pending or running.
>
> Thanks.
>
> --
> tejun

Alex

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

#1649929

Hello,

cc'ing ACPI folks.

On Wed, May 24, 2017 at 10:16:02PM +0200, Alex Naidis wrote:
> 2017-05-24 22:01 GMT+02:00 Tejun Heo <tj@kernel.org>:
> > Hello, Alex.
> >
> > On Wed, May 24, 2017 at 09:58:19PM +0200, Alex Naidis wrote:
> >> Yeah, I agree, it is wrong to rely on work_busy() providing correct data.
> >> However sometimes it is useful to have an indicator like this to at least
> >> catch some cases where requeuing work would be obsolete.
> >> This applies for delayed work too.
> >
> > Can you elaborage on "requeueing work would be obsolate" a bit?
> Sure.
> The case that I mean is represented well by the usage of work_busy here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/acpi/bus.c#n534
> It checks work_busy() before requeuing the work, since in some cases
> it is obsolete
> to requeue the work if it is already pending or running.

So, there is no point in testing whether a work item is pending before
queeing it.  It's completely spurious.  The only part which makes
sense is if it doesn't want to queue a work item if it's already
running (but note that this isn't synchronized properly) for some
reason, but given that it can race with workqueue operation and
spuriously return false when the work item just started running, it
can't be for correctness.

Rafael, Len, can you please explain why work_busy() test is there?

Thanks.

-- 
tejun

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

#1649935

Hello,

2017-05-24 22:21 GMT+02:00 Tejun Heo <tj@kernel.org>:
> Hello,
>
> cc'ing ACPI folks.
>
> On Wed, May 24, 2017 at 10:16:02PM +0200, Alex Naidis wrote:
>> 2017-05-24 22:01 GMT+02:00 Tejun Heo <tj@kernel.org>:
>> > Hello, Alex.
>> >
>> > On Wed, May 24, 2017 at 09:58:19PM +0200, Alex Naidis wrote:
>> >> Yeah, I agree, it is wrong to rely on work_busy() providing correct data.
>> >> However sometimes it is useful to have an indicator like this to at least
>> >> catch some cases where requeuing work would be obsolete.
>> >> This applies for delayed work too.
>> >
>> > Can you elaborage on "requeueing work would be obsolate" a bit?
>> Sure.
>> The case that I mean is represented well by the usage of work_busy here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/acpi/bus.c#n534
>> It checks work_busy() before requeuing the work, since in some cases
>> it is obsolete
>> to requeue the work if it is already pending or running.
>
> So, there is no point in testing whether a work item is pending before
> queeing it.  It's completely spurious.  The only part which makes
> sense is if it doesn't want to queue a work item if it's already
> running (but note that this isn't synchronized properly) for some
> reason, but given that it can race with workqueue operation and
> spuriously return false when the work item just started running, it
> can't be for correctness.
>
what I thought is that might be a good idea to catch at least
some cases where the work is already running. It is obviously wrong
to rely on the corectness of the return value.

Do you agree here or would you suggest another
alternative here?

> Rafael, Len, can you please explain why work_busy() test is there?
>
> Thanks.
>
> --
> tejun

Thank you!
Alex

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web