* [PATCH v2 REPOST] fscache: Use only one fscache_object_cong_wait.
@ 2021-11-18 16:54 Sebastian Andrzej Siewior
2021-12-02 20:52 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-11-18 16:54 UTC (permalink / raw)
To: linux-cachefs, linux-kernel
Cc: David Howells, Tejun Heo, Gregor Beck, Thomas Gleixner
In the commit mentioned below, fscache was converted from slow-work to
workqueue. slow_work_enqueue() and slow_work_sleep_till_thread_needed()
did not use a per-CPU workqueue. They choose from two global waitqueues
depending on the SLOW_WORK_VERY_SLOW bit which was not set so it always
one waitqueue.
I can't find out how it is ensured that a waiter on certain CPU is woken
up be the other side. My guess is that the timeout in schedule_timeout()
ensures that it does not wait forever (or a random wake up).
fscache_object_sleep_till_congested() must be invoked from preemptible
context in order for schedule() to work. In this case this_cpu_ptr()
should complain with CONFIG_DEBUG_PREEMPT enabled except the thread is
bound to one CPU.
wake_up() wakes only one waiter and I'm not sure if it is guaranteed
that only one waiter exists.
Replace the per-CPU waitqueue with one global waitqueue.
Fixes: 8b8edefa2fffb ("fscache: convert object to use workqueue instead of slow-work")
Reported-by: Gregor Beck <gregor.beck@gmail.com>
Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Previous post
https://lore.kernel.org/all/20211029083839.xwwt7jgzru3kcpii@linutronix.de/
fs/fscache/internal.h | 1 -
fs/fscache/main.c | 6 ------
fs/fscache/object.c | 13 +++++--------
3 files changed, 5 insertions(+), 15 deletions(-)
--- a/fs/fscache/internal.h
+++ b/fs/fscache/internal.h
@@ -81,7 +81,6 @@ extern unsigned fscache_debug;
extern struct kobject *fscache_root;
extern struct workqueue_struct *fscache_object_wq;
extern struct workqueue_struct *fscache_op_wq;
-DECLARE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
extern unsigned int fscache_hash(unsigned int salt, unsigned int *data, unsigned int n);
--- a/fs/fscache/main.c
+++ b/fs/fscache/main.c
@@ -41,8 +41,6 @@ struct kobject *fscache_root;
struct workqueue_struct *fscache_object_wq;
struct workqueue_struct *fscache_op_wq;
-DEFINE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
-
/* these values serve as lower bounds, will be adjusted in fscache_init() */
static unsigned fscache_object_max_active = 4;
static unsigned fscache_op_max_active = 2;
@@ -138,7 +136,6 @@ unsigned int fscache_hash(unsigned int s
static int __init fscache_init(void)
{
unsigned int nr_cpus = num_possible_cpus();
- unsigned int cpu;
int ret;
fscache_object_max_active =
@@ -161,9 +158,6 @@ static int __init fscache_init(void)
if (!fscache_op_wq)
goto error_op_wq;
- for_each_possible_cpu(cpu)
- init_waitqueue_head(&per_cpu(fscache_object_cong_wait, cpu));
-
ret = fscache_proc_init();
if (ret < 0)
goto error_proc;
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -798,6 +798,8 @@ void fscache_object_destroy(struct fscac
}
EXPORT_SYMBOL(fscache_object_destroy);
+static DECLARE_WAIT_QUEUE_HEAD(fscache_object_cong_wait);
+
/*
* enqueue an object for metadata-type processing
*/
@@ -806,16 +808,12 @@ void fscache_enqueue_object(struct fscac
_enter("{OBJ%x}", object->debug_id);
if (fscache_get_object(object, fscache_obj_get_queue) >= 0) {
- wait_queue_head_t *cong_wq =
- &get_cpu_var(fscache_object_cong_wait);
if (queue_work(fscache_object_wq, &object->work)) {
if (fscache_object_congested())
- wake_up(cong_wq);
+ wake_up(&fscache_object_cong_wait);
} else
fscache_put_object(object, fscache_obj_put_queue);
-
- put_cpu_var(fscache_object_cong_wait);
}
}
@@ -833,16 +831,15 @@ void fscache_enqueue_object(struct fscac
*/
bool fscache_object_sleep_till_congested(signed long *timeoutp)
{
- wait_queue_head_t *cong_wq = this_cpu_ptr(&fscache_object_cong_wait);
DEFINE_WAIT(wait);
if (fscache_object_congested())
return true;
- add_wait_queue_exclusive(cong_wq, &wait);
+ add_wait_queue_exclusive(&fscache_object_cong_wait, &wait);
if (!fscache_object_congested())
*timeoutp = schedule_timeout(*timeoutp);
- finish_wait(cong_wq, &wait);
+ finish_wait(&fscache_object_cong_wait, &wait);
return fscache_object_congested();
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 REPOST] fscache: Use only one fscache_object_cong_wait.
2021-11-18 16:54 [PATCH v2 REPOST] fscache: Use only one fscache_object_cong_wait Sebastian Andrzej Siewior
@ 2021-12-02 20:52 ` Sebastian Andrzej Siewior
2021-12-13 15:10 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-12-02 20:52 UTC (permalink / raw)
To: linux-cachefs, linux-kernel
Cc: David Howells, Tejun Heo, Gregor Beck, Thomas Gleixner
On 2021-11-18 17:54:44 [+0100], To linux-cachefs@redhat.com wrote:
> In the commit mentioned below, fscache was converted from slow-work to
> workqueue. slow_work_enqueue() and slow_work_sleep_till_thread_needed()
> did not use a per-CPU workqueue. They choose from two global waitqueues
> depending on the SLOW_WORK_VERY_SLOW bit which was not set so it always
> one waitqueue.
>
> I can't find out how it is ensured that a waiter on certain CPU is woken
> up be the other side. My guess is that the timeout in schedule_timeout()
> ensures that it does not wait forever (or a random wake up).
>
> fscache_object_sleep_till_congested() must be invoked from preemptible
> context in order for schedule() to work. In this case this_cpu_ptr()
> should complain with CONFIG_DEBUG_PREEMPT enabled except the thread is
> bound to one CPU.
>
> wake_up() wakes only one waiter and I'm not sure if it is guaranteed
> that only one waiter exists.
>
> Replace the per-CPU waitqueue with one global waitqueue.
>
> Fixes: 8b8edefa2fffb ("fscache: convert object to use workqueue instead of slow-work")
> Reported-by: Gregor Beck <gregor.beck@gmail.com>
> Cc: stable-rt@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
I noticed that -next gained commit
608bfec640edb ("fscache: Remove the contents of the fscache driver, pending rewrite")
which removes slow_work_sleep_till_thread_needed() and the per-CPU
variable. Since it looks like a bug, what happens stable wise?
> ---
> Previous post
> https://lore.kernel.org/all/20211029083839.xwwt7jgzru3kcpii@linutronix.de/
>
> fs/fscache/internal.h | 1 -
> fs/fscache/main.c | 6 ------
> fs/fscache/object.c | 13 +++++--------
> 3 files changed, 5 insertions(+), 15 deletions(-)
>
> --- a/fs/fscache/internal.h
> +++ b/fs/fscache/internal.h
> @@ -81,7 +81,6 @@ extern unsigned fscache_debug;
> extern struct kobject *fscache_root;
> extern struct workqueue_struct *fscache_object_wq;
> extern struct workqueue_struct *fscache_op_wq;
> -DECLARE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
>
> extern unsigned int fscache_hash(unsigned int salt, unsigned int *data, unsigned int n);
>
> --- a/fs/fscache/main.c
> +++ b/fs/fscache/main.c
> @@ -41,8 +41,6 @@ struct kobject *fscache_root;
> struct workqueue_struct *fscache_object_wq;
> struct workqueue_struct *fscache_op_wq;
>
> -DEFINE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
> -
> /* these values serve as lower bounds, will be adjusted in fscache_init() */
> static unsigned fscache_object_max_active = 4;
> static unsigned fscache_op_max_active = 2;
> @@ -138,7 +136,6 @@ unsigned int fscache_hash(unsigned int s
> static int __init fscache_init(void)
> {
> unsigned int nr_cpus = num_possible_cpus();
> - unsigned int cpu;
> int ret;
>
> fscache_object_max_active =
> @@ -161,9 +158,6 @@ static int __init fscache_init(void)
> if (!fscache_op_wq)
> goto error_op_wq;
>
> - for_each_possible_cpu(cpu)
> - init_waitqueue_head(&per_cpu(fscache_object_cong_wait, cpu));
> -
> ret = fscache_proc_init();
> if (ret < 0)
> goto error_proc;
> --- a/fs/fscache/object.c
> +++ b/fs/fscache/object.c
> @@ -798,6 +798,8 @@ void fscache_object_destroy(struct fscac
> }
> EXPORT_SYMBOL(fscache_object_destroy);
>
> +static DECLARE_WAIT_QUEUE_HEAD(fscache_object_cong_wait);
> +
> /*
> * enqueue an object for metadata-type processing
> */
> @@ -806,16 +808,12 @@ void fscache_enqueue_object(struct fscac
> _enter("{OBJ%x}", object->debug_id);
>
> if (fscache_get_object(object, fscache_obj_get_queue) >= 0) {
> - wait_queue_head_t *cong_wq =
> - &get_cpu_var(fscache_object_cong_wait);
>
> if (queue_work(fscache_object_wq, &object->work)) {
> if (fscache_object_congested())
> - wake_up(cong_wq);
> + wake_up(&fscache_object_cong_wait);
> } else
> fscache_put_object(object, fscache_obj_put_queue);
> -
> - put_cpu_var(fscache_object_cong_wait);
> }
> }
>
> @@ -833,16 +831,15 @@ void fscache_enqueue_object(struct fscac
> */
> bool fscache_object_sleep_till_congested(signed long *timeoutp)
> {
> - wait_queue_head_t *cong_wq = this_cpu_ptr(&fscache_object_cong_wait);
> DEFINE_WAIT(wait);
>
> if (fscache_object_congested())
> return true;
>
> - add_wait_queue_exclusive(cong_wq, &wait);
> + add_wait_queue_exclusive(&fscache_object_cong_wait, &wait);
> if (!fscache_object_congested())
> *timeoutp = schedule_timeout(*timeoutp);
> - finish_wait(cong_wq, &wait);
> + finish_wait(&fscache_object_cong_wait, &wait);
>
> return fscache_object_congested();
> }
Sebastian
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 REPOST] fscache: Use only one fscache_object_cong_wait.
2021-12-02 20:52 ` Sebastian Andrzej Siewior
@ 2021-12-13 15:10 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 3+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-12-13 15:10 UTC (permalink / raw)
To: linux-cachefs, linux-kernel
Cc: David Howells, Tejun Heo, Gregor Beck, Thomas Gleixner
On 2021-12-02 21:52:45 [+0100], To linux-cachefs@redhat.com wrote:
> On 2021-11-18 17:54:44 [+0100], To linux-cachefs@redhat.com wrote:
> > In the commit mentioned below, fscache was converted from slow-work to
> > workqueue. slow_work_enqueue() and slow_work_sleep_till_thread_needed()
> > did not use a per-CPU workqueue. They choose from two global waitqueues
> > depending on the SLOW_WORK_VERY_SLOW bit which was not set so it always
> > one waitqueue.
> >
> > I can't find out how it is ensured that a waiter on certain CPU is woken
> > up be the other side. My guess is that the timeout in schedule_timeout()
> > ensures that it does not wait forever (or a random wake up).
> >
> > fscache_object_sleep_till_congested() must be invoked from preemptible
> > context in order for schedule() to work. In this case this_cpu_ptr()
> > should complain with CONFIG_DEBUG_PREEMPT enabled except the thread is
> > bound to one CPU.
> >
> > wake_up() wakes only one waiter and I'm not sure if it is guaranteed
> > that only one waiter exists.
> >
> > Replace the per-CPU waitqueue with one global waitqueue.
> >
> > Fixes: 8b8edefa2fffb ("fscache: convert object to use workqueue instead of slow-work")
> > Reported-by: Gregor Beck <gregor.beck@gmail.com>
> > Cc: stable-rt@vger.kernel.org
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> I noticed that -next gained commit
> 608bfec640edb ("fscache: Remove the contents of the fscache driver, pending rewrite")
>
> which removes slow_work_sleep_till_thread_needed() and the per-CPU
> variable. Since it looks like a bug, what happens stable wise?
a gentle ping.
> > ---
> > Previous post
> > https://lore.kernel.org/all/20211029083839.xwwt7jgzru3kcpii@linutronix.de/
> >
> > fs/fscache/internal.h | 1 -
> > fs/fscache/main.c | 6 ------
> > fs/fscache/object.c | 13 +++++--------
> > 3 files changed, 5 insertions(+), 15 deletions(-)
> >
> > --- a/fs/fscache/internal.h
> > +++ b/fs/fscache/internal.h
> > @@ -81,7 +81,6 @@ extern unsigned fscache_debug;
> > extern struct kobject *fscache_root;
> > extern struct workqueue_struct *fscache_object_wq;
> > extern struct workqueue_struct *fscache_op_wq;
> > -DECLARE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
> >
> > extern unsigned int fscache_hash(unsigned int salt, unsigned int *data, unsigned int n);
> >
> > --- a/fs/fscache/main.c
> > +++ b/fs/fscache/main.c
> > @@ -41,8 +41,6 @@ struct kobject *fscache_root;
> > struct workqueue_struct *fscache_object_wq;
> > struct workqueue_struct *fscache_op_wq;
> >
> > -DEFINE_PER_CPU(wait_queue_head_t, fscache_object_cong_wait);
> > -
> > /* these values serve as lower bounds, will be adjusted in fscache_init() */
> > static unsigned fscache_object_max_active = 4;
> > static unsigned fscache_op_max_active = 2;
> > @@ -138,7 +136,6 @@ unsigned int fscache_hash(unsigned int s
> > static int __init fscache_init(void)
> > {
> > unsigned int nr_cpus = num_possible_cpus();
> > - unsigned int cpu;
> > int ret;
> >
> > fscache_object_max_active =
> > @@ -161,9 +158,6 @@ static int __init fscache_init(void)
> > if (!fscache_op_wq)
> > goto error_op_wq;
> >
> > - for_each_possible_cpu(cpu)
> > - init_waitqueue_head(&per_cpu(fscache_object_cong_wait, cpu));
> > -
> > ret = fscache_proc_init();
> > if (ret < 0)
> > goto error_proc;
> > --- a/fs/fscache/object.c
> > +++ b/fs/fscache/object.c
> > @@ -798,6 +798,8 @@ void fscache_object_destroy(struct fscac
> > }
> > EXPORT_SYMBOL(fscache_object_destroy);
> >
> > +static DECLARE_WAIT_QUEUE_HEAD(fscache_object_cong_wait);
> > +
> > /*
> > * enqueue an object for metadata-type processing
> > */
> > @@ -806,16 +808,12 @@ void fscache_enqueue_object(struct fscac
> > _enter("{OBJ%x}", object->debug_id);
> >
> > if (fscache_get_object(object, fscache_obj_get_queue) >= 0) {
> > - wait_queue_head_t *cong_wq =
> > - &get_cpu_var(fscache_object_cong_wait);
> >
> > if (queue_work(fscache_object_wq, &object->work)) {
> > if (fscache_object_congested())
> > - wake_up(cong_wq);
> > + wake_up(&fscache_object_cong_wait);
> > } else
> > fscache_put_object(object, fscache_obj_put_queue);
> > -
> > - put_cpu_var(fscache_object_cong_wait);
> > }
> > }
> >
> > @@ -833,16 +831,15 @@ void fscache_enqueue_object(struct fscac
> > */
> > bool fscache_object_sleep_till_congested(signed long *timeoutp)
> > {
> > - wait_queue_head_t *cong_wq = this_cpu_ptr(&fscache_object_cong_wait);
> > DEFINE_WAIT(wait);
> >
> > if (fscache_object_congested())
> > return true;
> >
> > - add_wait_queue_exclusive(cong_wq, &wait);
> > + add_wait_queue_exclusive(&fscache_object_cong_wait, &wait);
> > if (!fscache_object_congested())
> > *timeoutp = schedule_timeout(*timeoutp);
> > - finish_wait(cong_wq, &wait);
> > + finish_wait(&fscache_object_cong_wait, &wait);
> >
> > return fscache_object_congested();
> > }
Sebastian
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-13 15:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 16:54 [PATCH v2 REPOST] fscache: Use only one fscache_object_cong_wait Sebastian Andrzej Siewior
2021-12-02 20:52 ` Sebastian Andrzej Siewior
2021-12-13 15:10 ` Sebastian Andrzej Siewior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.