On Sat, Dec 19, 2020 at 1:59 AM Valentin Schneider <valentin.schnei...@arm.com> wrote: > > > On 18/12/20 17:09, Lai Jiangshan wrote: > > From: Lai Jiangshan <la...@linux.alibaba.com> > > > > When worker_attach_to_pool() is called, we should not put the workers > > to pool->attrs->cpumask when there is not CPU online in it. > > > > We have to use wq_online_cpumask in worker_attach_to_pool() to check > > if pool->attrs->cpumask is valid rather than cpu_online_mask or > > cpu_active_mask due to gaps between stages in cpu hot[un]plug. > > > > So for that late-spawned per-CPU kworker case: the outgoing CPU should have > > already been cleared from wq_online_cpumask, so it gets its affinity reset > > to the possible mask and the subsequent wakeup will ensure it's put on an > > active CPU. > > > > To use wq_online_cpumask in worker_attach_to_pool(), we need to protect > > wq_online_cpumask in wq_pool_attach_mutex and we modify > > workqueue_online_cpu() > > and workqueue_offline_cpu() to enlarge wq_pool_attach_mutex protected > > region. We also put updating wq_online_cpumask and [re|un]bind_workers() > > in the same wq_pool_attach_mutex protected region to make the update > > for percpu workqueue atomically. > > > > Cc: Qian Cai <c...@redhat.com> > > Cc: Peter Zijlstra <pet...@infradead.org> > > Cc: Vincent Donnefort <vincent.donnef...@arm.com> > > Link: > > https://lore.kernel.org/lkml/20201210163830.21514-3-valentin.schnei...@arm.com/ > > Acked-by: Valentin Schneider <valentin.schnei...@arm.com> > > So an etiquette thing: I never actually gave an Acked-by. I did say it > looked good to me, and that probably should've been bundled with a > Reviewed-by, but it wasn't (I figured I'd wait for v2). Forging is bad, > m'kay. > > When in doubt (e.g. someone says they're ok with your patch but don't give > any Ack/Reviewed-by), just ask via mail or on IRC.
Hello, Valentin I'm sorry not to have asked for your option. When I saw "Seems alright to me." I felt a huge encouragement and rushed. I was in doubt should I promote "Seems alright to me." to "Ack". Instead of asking, I wrongly did it right the way. I knew may I'm just forging, and added a log in the cover letter: > Add Valentin's ack for patch 10 because "Seems alright to me." and > add Valentin's comments to the changelog which is integral. Anyway, it is my bad and I learnt. > > For now, please make this a: > > Reviewed-by: Valentin Schneider <valentin.schnei...@arm.com> Hello Peter, cloud you help change it if there is no other feedback that causes V3 patchset to be made. Thanks Lai > > > Acked-by: Tejun Heo <t...@kernel.org> > > Signed-off-by: Lai Jiangshan <la...@linux.alibaba.com> > > --- > > kernel/workqueue.c | 32 +++++++++++++++----------------- > > 1 file changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > > index 65270729454c..eeb726598f80 100644 > > --- a/kernel/workqueue.c > > +++ b/kernel/workqueue.c > > @@ -310,7 +310,7 @@ static bool workqueue_freezing; /* PL: have > > wqs started freezing? */ > > /* PL: allowable cpus for unbound wqs and work items */ > > static cpumask_var_t wq_unbound_cpumask; > > > > -/* PL: online cpus (cpu_online_mask with the going-down cpu cleared) */ > > +/* PL&A: online cpus (cpu_online_mask with the going-down cpu cleared) */ > > static cpumask_var_t wq_online_cpumask; > > > > /* CPU where unbound work was last round robin scheduled from this CPU */ > > @@ -1848,11 +1848,11 @@ static void worker_attach_to_pool(struct worker > > *worker, > > { > > mutex_lock(&wq_pool_attach_mutex); > > > > - /* > > - * set_cpus_allowed_ptr() will fail if the cpumask doesn't have any > > - * online CPUs. It'll be re-applied when any of the CPUs come up. > > - */ > > - set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask); > > + /* Is there any cpu in pool->attrs->cpumask online? */ > > + if (cpumask_intersects(pool->attrs->cpumask, wq_online_cpumask)) > > + WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, > > pool->attrs->cpumask) < 0); > > + else > > + WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, > > cpu_possible_mask) < 0); > > > > /* > > * The wq_pool_attach_mutex ensures %POOL_DISASSOCIATED remains > > @@ -5081,13 +5081,12 @@ int workqueue_online_cpu(unsigned int cpu) > > int pi; > > > > mutex_lock(&wq_pool_mutex); > > - cpumask_set_cpu(cpu, wq_online_cpumask); > > > > - for_each_cpu_worker_pool(pool, cpu) { > > - mutex_lock(&wq_pool_attach_mutex); > > + mutex_lock(&wq_pool_attach_mutex); > > + cpumask_set_cpu(cpu, wq_online_cpumask); > > + for_each_cpu_worker_pool(pool, cpu) > > rebind_workers(pool); > > - mutex_unlock(&wq_pool_attach_mutex); > > - } > > + mutex_unlock(&wq_pool_attach_mutex); > > > > /* update CPU affinity of workers of unbound pools */ > > for_each_pool(pool, pi) { > > @@ -5117,14 +5116,13 @@ int workqueue_offline_cpu(unsigned int cpu) > > if (WARN_ON(cpu != smp_processor_id())) > > return -1; > > > > - for_each_cpu_worker_pool(pool, cpu) { > > - mutex_lock(&wq_pool_attach_mutex); > > - unbind_workers(pool); > > - mutex_unlock(&wq_pool_attach_mutex); > > - } > > - > > mutex_lock(&wq_pool_mutex); > > + > > + mutex_lock(&wq_pool_attach_mutex); > > cpumask_clear_cpu(cpu, wq_online_cpumask); > > + for_each_cpu_worker_pool(pool, cpu) > > + unbind_workers(pool); > > + mutex_unlock(&wq_pool_attach_mutex); > > > > /* update CPU affinity of workers of unbound pools */ > > for_each_pool(pool, pi) {