Messages in this thread | | | Date | Tue, 11 Mar 2014 15:03:23 +0100 | From | Alexander Gordeev <> | Subject | Re: [PATCH RESEND 1/3] percpu_ida: Fix data race on cpus_have_tags cpumask |
| |
On Sun, Mar 02, 2014 at 10:42:05PM +0800, Ming Lei wrote: > >> @@ -237,8 +242,11 @@ void percpu_ida_free(struct percpu_ida *pool, unsigned tag) > >> spin_unlock(&tags->lock); > >> > >> if (nr_free == 1) { > >> - cpumask_set_cpu(smp_processor_id(), > >> - &pool->cpus_have_tags); > >> + cpumask_set_cpu(smp_processor_id(), &pool->cpus_have_tags); > >> + /* > >> + * Pairs with smp_rmb() in steal_tags() > >> + */ > >> + smp_wmb(); > >> wake_up(&pool->wait); > > > > I think I'm nacking this - there's a lot of code in the kernel that relies on > > the fact that prepare_to_wait)/wake_up() do the appropriate fences, we really > > shouldn't be adding to the barriers those do. > > In theory, it still might cause percpu_ida_alloc(TASK_RUNNING) failed, > looks it isn't a big deal for the case. > > But I am wondering why cpumask_set_cpu() isn't called with > holding lock inside percpu_ida_free()? Looks 'nr_free == 1' > shouldn't have happened frequently.
Because bouncing on the lock is more expensive than occasionally putting a thread into sleep.
> > > Thanks, > -- > Ming Lei
-- Regards, Alexander Gordeev agordeev@redhat.com
| |