lkml.org 
[lkml]   [2019]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] psi: fix aggregation idle shut-off
    On Wed, 16 Jan 2019 14:35:01 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:

    > psi has provisions to shut off the periodic aggregation worker when
    > there is a period of no task activity - and thus no data that needs
    > aggregating. However, while developing psi monitoring, Suren noticed
    > that the aggregation clock currently won't stay shut off for good.
    >
    > Debugging this revealed a flaw in the idle design: an aggregation run
    > will see no task activity and decide to go to sleep; shortly
    > thereafter, the kworker thread that executed the aggregation will go
    > idle and cause a scheduling change, during which the psi callback will
    > kick the !pending worker again. This will ping-pong forever, and is
    > equivalent to having no shut-off logic at all (but with more code!)
    >
    > Fix this by exempting aggregation workers from psi's clock waking
    > logic when the state change is them going to sleep. To do this, tag
    > workers with the last work function they executed, and if in psi we
    > see a worker going to sleep after aggregating psi data, we will not
    > reschedule the aggregation work item.
    >
    > What if the worker is also executing other items before or after?
    >
    > Any psi state times that were incurred by work items preceding the
    > aggregation work will have been collected from the per-cpu buckets
    > during the aggregation itself. If there are work items following the
    > aggregation work, the worker's last_func tag will be overwritten and
    > the aggregator will be kept alive to process this genuine new activity.
    >
    > If the aggregation work is the last thing the worker does, and we
    > decide to go idle, the brief period of non-idle time incurred between
    > the aggregation run and the kworker's dequeue will be stranded in the
    > per-cpu buckets until the clock is woken by later activity. But that
    > should not be a problem. The buckets can hold 4s worth of time, and
    > future activity will wake the clock with a 2s delay, giving us 2s
    > worth of data we can leave behind when disabling aggregation. If it
    > takes a worker more than two seconds to go idle after it finishes its
    > last work item, we likely have bigger problems in the system, and
    > won't notice one sample that was averaged with a bogus per-CPU weight.

    Did we ever hear back from Suren about the testing?

    Some words here about the new wq_worker_last_func() would be
    appropriate.

    It's an ugly-looking thing :( Tejun, did you check this change?


    > --- a/kernel/sched/psi.c
    > +++ b/kernel/sched/psi.c
    > @@ -124,6 +124,7 @@
    > * sampling of the aggregate task states would be.
    > */
    >
    > +#include "../workqueue_internal.h"

    "Only to be included by workqueue and core kernel subsystems"

    I'm not sure that psi qualifies. Perhaps wq_worker_last_func() should
    be declared in include/linux/workqueue.h.

    And perhaps implemented there as well. It's similar to
    current_wq_worker(), which is inlined and wq_worker_last_func() is
    small enough to justify inlining.

    > --- a/kernel/workqueue.c
    > +++ b/kernel/workqueue.c
    > @@ -909,6 +909,26 @@ struct task_struct *wq_worker_sleeping(struct task_struct *task)
    > return to_wakeup ? to_wakeup->task : NULL;
    > }
    >
    > +/**
    > + * wq_worker_last_func - retrieve worker's last work function
    > + *
    > + * Determine the last function a worker executed. This is called from
    > + * the scheduler to get a worker's last known identity.
    > + *
    > + * CONTEXT:
    > + * spin_lock_irq(rq->lock)
    > + *
    > + * Return:
    > + * The last work function %current executed as a worker, NULL if it
    > + * hasn't executed any work yet.
    > + */
    > +work_func_t wq_worker_last_func(struct task_struct *task)
    > +{
    > + struct worker *worker = kthread_data(task);
    > +
    > + return worker->last_func;
    > +}

    The semantics are troublesome. What guarantees that worker->last_func
    won't change under the caller's feet? The caller should hold some lock
    (presumably worker->pool->lock) in order to stabilize the
    wq_worker_last_func() return value?

    Also, the comment isn't really true - this is called from PSI, which is
    hardly "the scheduler"?


    \
     
     \ /
      Last update: 2019-01-28 23:04    [W:2.392 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site