Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
On 2020/11/19 9:58, Lai Jiangshan wrote: > On Wed, Nov 18, 2020 at 5:05 PM Yunfeng Ye wrote: >> >> >> >> On 2020/11/18 14:26, Yunfeng Ye wrote: >>> >>> >>> On 2020/11/18 12:06, Lai Jiangshan wrote: >>>> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye wrote: >>>>> >>>>> In realtime scenario, We do not want to have interference on the >>>>> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq >>>>> on the housekeeping cpu, it kick a kworker on the isolated cpu. >>>>> >>>>> alloc_workqueue >>>>> pwq_adjust_max_active >>>>> wake_up_worker >>>>> >>>>> The comment in pwq_adjust_max_active() said: >>>>> "Need to kick a worker after thawed or an unbound wq's >>>>>max_active is bumped" >>>>> >>>>> So it is unnecessary to kick a kworker for percpu wq's when >>>>> alloc_workqueue. this patch only kick a worker after thawed or for an >>>>> unbound workqueue. >>>>> >>>>> Signed-off-by: Yunfeng Ye >>>>> --- >>>>> kernel/workqueue.c | 18 +- >>>>> 1 file changed, 13 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c >>>>> index c41c3c17b86a..80f7bbd4889f 100644 >>>>> --- a/kernel/workqueue.c >>>>> +++ b/kernel/workqueue.c >>>>> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct >>>>> work_struct *work) >>>>> } >>>>> >>>>> /** >>>>> - * pwq_adjust_max_active - update a pwq's max_active to the current >>>>> setting >>>>> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the >>>>> current setting >>>>> * @pwq: target pool_workqueue >>>>> + * @force_kick: force to kick a worker >>>>> * >>>>> * If @pwq isn't freezing, set @pwq->max_active to the associated >>>>> * workqueue's saved_max_active and activate delayed work items >>>>> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero. >>>>> */ >>>>> -static void pwq_adjust_max_active(struct pool_workqueue *pwq) >>>>> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq, >>>>> + bool force_kick) >>>>> { >>>>> struct workqueue_struct *wq = pwq->wq; >>>>> bool freezable = wq->flags & WQ_FREEZABLE; >>>>> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct >>>>> pool_workqueue *pwq) >>>>> >>>>> /* >>>>> * Need to kick a worker after thawed or an unbound wq's >>>>> -* max_active is bumped. It's a slow path. Do it always. >>>>> +* max_active is bumped. >>>> >>>> >>>> Hello >>>> >>>> Thanks for reporting the problem. >>>> >>>> But I don't like to add an argument. The waking up is called >>>> always just because it was considered no harm and it is slow >>>> path. But it can still be possible to detect if the waking up >>>> is really needed based on the actual activation of delayed works. >>>> >>>> The previous lines are: >>>> >>>> while (!list_empty(>delayed_works) && >>>>pwq->nr_active < pwq->max_active) >>>> pwq_activate_first_delayed(pwq); >>>> >>>> And you can record the old pwq->nr_active before these lines: >>>> >>>> int old_nr_active = pwq->nr_active; >>>> >>>> while (!list_empty(>delayed_works) && >>>>pwq->nr_active < pwq->max_active) >>>> pwq_activate_first_delayed(pwq); >>>> >>>> /* please add more comments here, see 951a078a5 */ >>>> if (old_nr_active < pwq->nr_active) { >>>> if (!old_nr_active || (wq->flags & WQ_UNBOUND)) >>>> wa
Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
On Wed, Nov 18, 2020 at 5:05 PM Yunfeng Ye wrote: > > > > On 2020/11/18 14:26, Yunfeng Ye wrote: > > > > > > On 2020/11/18 12:06, Lai Jiangshan wrote: > >> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye wrote: > >>> > >>> In realtime scenario, We do not want to have interference on the > >>> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq > >>> on the housekeeping cpu, it kick a kworker on the isolated cpu. > >>> > >>> alloc_workqueue > >>> pwq_adjust_max_active > >>> wake_up_worker > >>> > >>> The comment in pwq_adjust_max_active() said: > >>> "Need to kick a worker after thawed or an unbound wq's > >>>max_active is bumped" > >>> > >>> So it is unnecessary to kick a kworker for percpu wq's when > >>> alloc_workqueue. this patch only kick a worker after thawed or for an > >>> unbound workqueue. > >>> > >>> Signed-off-by: Yunfeng Ye > >>> --- > >>> kernel/workqueue.c | 18 +- > >>> 1 file changed, 13 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c > >>> index c41c3c17b86a..80f7bbd4889f 100644 > >>> --- a/kernel/workqueue.c > >>> +++ b/kernel/workqueue.c > >>> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct > >>> work_struct *work) > >>> } > >>> > >>> /** > >>> - * pwq_adjust_max_active - update a pwq's max_active to the current > >>> setting > >>> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the > >>> current setting > >>> * @pwq: target pool_workqueue > >>> + * @force_kick: force to kick a worker > >>> * > >>> * If @pwq isn't freezing, set @pwq->max_active to the associated > >>> * workqueue's saved_max_active and activate delayed work items > >>> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero. > >>> */ > >>> -static void pwq_adjust_max_active(struct pool_workqueue *pwq) > >>> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq, > >>> + bool force_kick) > >>> { > >>> struct workqueue_struct *wq = pwq->wq; > >>> bool freezable = wq->flags & WQ_FREEZABLE; > >>> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct > >>> pool_workqueue *pwq) > >>> > >>> /* > >>> * Need to kick a worker after thawed or an unbound wq's > >>> -* max_active is bumped. It's a slow path. Do it always. > >>> +* max_active is bumped. > >> > >> > >> Hello > >> > >> Thanks for reporting the problem. > >> > >> But I don't like to add an argument. The waking up is called > >> always just because it was considered no harm and it is slow > >> path. But it can still be possible to detect if the waking up > >> is really needed based on the actual activation of delayed works. > >> > >> The previous lines are: > >> > >> while (!list_empty(>delayed_works) && > >>pwq->nr_active < pwq->max_active) > >> pwq_activate_first_delayed(pwq); > >> > >> And you can record the old pwq->nr_active before these lines: > >> > >> int old_nr_active = pwq->nr_active; > >> > >> while (!list_empty(>delayed_works) && > >>pwq->nr_active < pwq->max_active) > >> pwq_activate_first_delayed(pwq); > >> > >> /* please add more comments here, see 951a078a5 */ > >> if (old_nr_active < pwq->nr_active) { > >> if (!old_nr_active || (wq->flags & WQ_UNBOUND)) > >> wake_up_worker(pwq->pool); > >> } > >> > > Ok, I will send a patch v2. > > Thanks. > > > I think it is unnecessary to distinguish the percpu or unbound's wq, > kick a worker always based on the actual activation of delayed works. > > Look like this: > > diff --git a
Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
On 2020/11/18 14:26, Yunfeng Ye wrote: > > > On 2020/11/18 12:06, Lai Jiangshan wrote: >> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye wrote: >>> >>> In realtime scenario, We do not want to have interference on the >>> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq >>> on the housekeeping cpu, it kick a kworker on the isolated cpu. >>> >>> alloc_workqueue >>> pwq_adjust_max_active >>> wake_up_worker >>> >>> The comment in pwq_adjust_max_active() said: >>> "Need to kick a worker after thawed or an unbound wq's >>> max_active is bumped" >>> >>> So it is unnecessary to kick a kworker for percpu wq's when >>> alloc_workqueue. this patch only kick a worker after thawed or for an >>> unbound workqueue. >>> >>> Signed-off-by: Yunfeng Ye >>> --- >>> kernel/workqueue.c | 18 +- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c >>> index c41c3c17b86a..80f7bbd4889f 100644 >>> --- a/kernel/workqueue.c >>> +++ b/kernel/workqueue.c >>> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct >>> work_struct *work) >>> } >>> >>> /** >>> - * pwq_adjust_max_active - update a pwq's max_active to the current setting >>> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the >>> current setting >>> * @pwq: target pool_workqueue >>> + * @force_kick: force to kick a worker >>> * >>> * If @pwq isn't freezing, set @pwq->max_active to the associated >>> * workqueue's saved_max_active and activate delayed work items >>> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero. >>> */ >>> -static void pwq_adjust_max_active(struct pool_workqueue *pwq) >>> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq, >>> + bool force_kick) >>> { >>> struct workqueue_struct *wq = pwq->wq; >>> bool freezable = wq->flags & WQ_FREEZABLE; >>> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct >>> pool_workqueue *pwq) >>> >>> /* >>> * Need to kick a worker after thawed or an unbound wq's >>> -* max_active is bumped. It's a slow path. Do it always. >>> +* max_active is bumped. >> >> >> Hello >> >> Thanks for reporting the problem. >> >> But I don't like to add an argument. The waking up is called >> always just because it was considered no harm and it is slow >> path. But it can still be possible to detect if the waking up >> is really needed based on the actual activation of delayed works. >> >> The previous lines are: >> >> while (!list_empty(>delayed_works) && >>pwq->nr_active < pwq->max_active) >> pwq_activate_first_delayed(pwq); >> >> And you can record the old pwq->nr_active before these lines: >> >> int old_nr_active = pwq->nr_active; >> >> while (!list_empty(>delayed_works) && >>pwq->nr_active < pwq->max_active) >> pwq_activate_first_delayed(pwq); >> >> /* please add more comments here, see 951a078a5 */ >> if (old_nr_active < pwq->nr_active) { >> if (!old_nr_active || (wq->flags & WQ_UNBOUND)) >> wake_up_worker(pwq->pool); >> } >> > Ok, I will send a patch v2. > Thanks. > I think it is unnecessary to distinguish the percpu or unbound's wq, kick a worker always based on the actual activation of delayed works. Look like this: diff --git a/kernel/workqueue.c b/kernel/workqueue.c index c41c3c17b86a..cd551dcb2cc9 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -3725,17 +3725,23 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq) * is updated and visible. */ if (!freezable || !workqueue_freezing) { + bool kick = false; + pwq->max_active = wq->saved_max_active; while (!list_empty(>delayed_works) && - pwq->nr_active < pwq->max_active)
Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
On 2020/11/18 12:06, Lai Jiangshan wrote: > On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye wrote: >> >> In realtime scenario, We do not want to have interference on the >> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq >> on the housekeeping cpu, it kick a kworker on the isolated cpu. >> >> alloc_workqueue >> pwq_adjust_max_active >> wake_up_worker >> >> The comment in pwq_adjust_max_active() said: >> "Need to kick a worker after thawed or an unbound wq's >>max_active is bumped" >> >> So it is unnecessary to kick a kworker for percpu wq's when >> alloc_workqueue. this patch only kick a worker after thawed or for an >> unbound workqueue. >> >> Signed-off-by: Yunfeng Ye >> --- >> kernel/workqueue.c | 18 +- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/kernel/workqueue.c b/kernel/workqueue.c >> index c41c3c17b86a..80f7bbd4889f 100644 >> --- a/kernel/workqueue.c >> +++ b/kernel/workqueue.c >> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct >> work_struct *work) >> } >> >> /** >> - * pwq_adjust_max_active - update a pwq's max_active to the current setting >> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the >> current setting >> * @pwq: target pool_workqueue >> + * @force_kick: force to kick a worker >> * >> * If @pwq isn't freezing, set @pwq->max_active to the associated >> * workqueue's saved_max_active and activate delayed work items >> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero. >> */ >> -static void pwq_adjust_max_active(struct pool_workqueue *pwq) >> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq, >> + bool force_kick) >> { >> struct workqueue_struct *wq = pwq->wq; >> bool freezable = wq->flags & WQ_FREEZABLE; >> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct >> pool_workqueue *pwq) >> >> /* >> * Need to kick a worker after thawed or an unbound wq's >> -* max_active is bumped. It's a slow path. Do it always. >> +* max_active is bumped. > > > Hello > > Thanks for reporting the problem. > > But I don't like to add an argument. The waking up is called > always just because it was considered no harm and it is slow > path. But it can still be possible to detect if the waking up > is really needed based on the actual activation of delayed works. > > The previous lines are: > > while (!list_empty(>delayed_works) && >pwq->nr_active < pwq->max_active) > pwq_activate_first_delayed(pwq); > > And you can record the old pwq->nr_active before these lines: > > int old_nr_active = pwq->nr_active; > > while (!list_empty(>delayed_works) && >pwq->nr_active < pwq->max_active) > pwq_activate_first_delayed(pwq); > > /* please add more comments here, see 951a078a5 */ > if (old_nr_active < pwq->nr_active) { > if (!old_nr_active || (wq->flags & WQ_UNBOUND)) > wake_up_worker(pwq->pool); > } > Ok, I will send a patch v2. Thanks. > > Thanks for your work. > Lai. > >> */ >> - wake_up_worker(pwq->pool); >> + if (force_kick || (wq->flags & WQ_UNBOUND)) >> + wake_up_worker(pwq->pool); >> } else { >> pwq->max_active = 0; >> } >> @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct >> pool_workqueue *pwq) >> raw_spin_unlock_irqrestore(>pool->lock, flags); >> } >> >> +static void pwq_adjust_max_active(struct pool_workqueue *pwq) >> +{ >> + pwq_adjust_max_active_and_kick(pwq, false); >> +} >> + >> /* initialize newly alloced @pwq which is associated with @wq and @pool */ >> static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct >> *wq, >> struct worker_pool *pool) >> @@ -5252,7 +5260,7 @@ void thaw_workqueues(void) >> list_for_each_entry(wq, , list) { >> mutex_lock(>mutex); >> for_each_pwq(pwq, wq) >> - pwq_adjust_max_active(pwq); >> + pwq_adjust_max_active_and_kick(pwq, true); >> mutex_unlock(>mutex); >> } >> >> -- >> 2.18.4 > . >
Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye wrote: > > In realtime scenario, We do not want to have interference on the > isolated cpu cores. but when invoking alloc_workqueue() for percpu wq > on the housekeeping cpu, it kick a kworker on the isolated cpu. > > alloc_workqueue > pwq_adjust_max_active > wake_up_worker > > The comment in pwq_adjust_max_active() said: > "Need to kick a worker after thawed or an unbound wq's >max_active is bumped" > > So it is unnecessary to kick a kworker for percpu wq's when > alloc_workqueue. this patch only kick a worker after thawed or for an > unbound workqueue. > > Signed-off-by: Yunfeng Ye > --- > kernel/workqueue.c | 18 +- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > index c41c3c17b86a..80f7bbd4889f 100644 > --- a/kernel/workqueue.c > +++ b/kernel/workqueue.c > @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct > work_struct *work) > } > > /** > - * pwq_adjust_max_active - update a pwq's max_active to the current setting > + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current > setting > * @pwq: target pool_workqueue > + * @force_kick: force to kick a worker > * > * If @pwq isn't freezing, set @pwq->max_active to the associated > * workqueue's saved_max_active and activate delayed work items > * accordingly. If @pwq is freezing, clear @pwq->max_active to zero. > */ > -static void pwq_adjust_max_active(struct pool_workqueue *pwq) > +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq, > + bool force_kick) > { > struct workqueue_struct *wq = pwq->wq; > bool freezable = wq->flags & WQ_FREEZABLE; > @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct > pool_workqueue *pwq) > > /* > * Need to kick a worker after thawed or an unbound wq's > -* max_active is bumped. It's a slow path. Do it always. > +* max_active is bumped. Hello Thanks for reporting the problem. But I don't like to add an argument. The waking up is called always just because it was considered no harm and it is slow path. But it can still be possible to detect if the waking up is really needed based on the actual activation of delayed works. The previous lines are: while (!list_empty(>delayed_works) && pwq->nr_active < pwq->max_active) pwq_activate_first_delayed(pwq); And you can record the old pwq->nr_active before these lines: int old_nr_active = pwq->nr_active; while (!list_empty(>delayed_works) && pwq->nr_active < pwq->max_active) pwq_activate_first_delayed(pwq); /* please add more comments here, see 951a078a5 */ if (old_nr_active < pwq->nr_active) { if (!old_nr_active || (wq->flags & WQ_UNBOUND)) wake_up_worker(pwq->pool); } Thanks for your work. Lai. > */ > - wake_up_worker(pwq->pool); > + if (force_kick || (wq->flags & WQ_UNBOUND)) > + wake_up_worker(pwq->pool); > } else { > pwq->max_active = 0; > } > @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct > pool_workqueue *pwq) > raw_spin_unlock_irqrestore(>pool->lock, flags); > } > > +static void pwq_adjust_max_active(struct pool_workqueue *pwq) > +{ > + pwq_adjust_max_active_and_kick(pwq, false); > +} > + > /* initialize newly alloced @pwq which is associated with @wq and @pool */ > static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq, > struct worker_pool *pool) > @@ -5252,7 +5260,7 @@ void thaw_workqueues(void) > list_for_each_entry(wq, , list) { > mutex_lock(>mutex); > for_each_pwq(pwq, wq) > - pwq_adjust_max_active(pwq); > + pwq_adjust_max_active_and_kick(pwq, true); > mutex_unlock(>mutex); > } > > -- > 2.18.4
workqueue: Only kick a worker after thawed or for an unbound workqueue
In realtime scenario, We do not want to have interference on the isolated cpu cores. but when invoking alloc_workqueue() for percpu wq on the housekeeping cpu, it kick a kworker on the isolated cpu. alloc_workqueue pwq_adjust_max_active wake_up_worker The comment in pwq_adjust_max_active() said: "Need to kick a worker after thawed or an unbound wq's max_active is bumped" So it is unnecessary to kick a kworker for percpu wq's when alloc_workqueue. this patch only kick a worker after thawed or for an unbound workqueue. Signed-off-by: Yunfeng Ye --- kernel/workqueue.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/kernel/workqueue.c b/kernel/workqueue.c index c41c3c17b86a..80f7bbd4889f 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work) } /** - * pwq_adjust_max_active - update a pwq's max_active to the current setting + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current setting * @pwq: target pool_workqueue + * @force_kick: force to kick a worker * * If @pwq isn't freezing, set @pwq->max_active to the associated * workqueue's saved_max_active and activate delayed work items * accordingly. If @pwq is freezing, clear @pwq->max_active to zero. */ -static void pwq_adjust_max_active(struct pool_workqueue *pwq) +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq, + bool force_kick) { struct workqueue_struct *wq = pwq->wq; bool freezable = wq->flags & WQ_FREEZABLE; @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq) /* * Need to kick a worker after thawed or an unbound wq's -* max_active is bumped. It's a slow path. Do it always. +* max_active is bumped. */ - wake_up_worker(pwq->pool); + if (force_kick || (wq->flags & WQ_UNBOUND)) + wake_up_worker(pwq->pool); } else { pwq->max_active = 0; } @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq) raw_spin_unlock_irqrestore(>pool->lock, flags); } +static void pwq_adjust_max_active(struct pool_workqueue *pwq) +{ + pwq_adjust_max_active_and_kick(pwq, false); +} + /* initialize newly alloced @pwq which is associated with @wq and @pool */ static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq, struct worker_pool *pool) @@ -5252,7 +5260,7 @@ void thaw_workqueues(void) list_for_each_entry(wq, , list) { mutex_lock(>mutex); for_each_pwq(pwq, wq) - pwq_adjust_max_active(pwq); + pwq_adjust_max_active_and_kick(pwq, true); mutex_unlock(>mutex); } -- 2.18.4