Re: [PATCH v3 1/3] lib/percpu-list: Per-cpu list with associated per-cpu locks

[Waiman Long]

On 02/23/2016 09:00 PM, Boqun Feng wrote:
> Hi Waiman,
>
> On Tue, Feb 23, 2016 at 02:04:30PM -0500, Waiman Long wrote:
> > }
> > +
> > +/*
> > + * List selection is based on the CPU being used when the pcpu_list_add()
> > + * function is called. However, deletion may be done by a different CPU.
> > + * So we still need to use a lock to protect the content of the list.
> > + */
> > +void pcpu_list_add(struct pcpu_list_node *node, struct pcpu_list_head *head)
> > +{
> > +	spinlock_t *lock;
> > +
> > +	/*
> > +	 * There is a very slight chance the cpu will be changed
> > +	 * (by preemption) before calling spin_lock(). We only need to put
> > +	 * the node in one of the per-cpu lists. It may not need to be
> > +	 * that of the current cpu.
> > +	 */
> Just curious about the comment here, what if the following happens:
>
> 	CPU 0				CPU 1
> 	=====================		=====================
> 	task_1:
>
> 	lock = this_cpu_ptr(&head->lock); // head->lock is on CPU0
> 	<preempted>
> 					continue to task_1:
> 					spin_lock(lock);
> 					node->lockptr = lock;
> 					// head->list is on CPU1
> 					list_add(&node->list, this_cpu_ptr(&head->list));
> 					spin_unlock(lock);
>
> , which ends up the node is in the list on CPU1 while ->lockptr pointing
> to the lock on CPU0.
>
> If there is another node whose ->lockptr points to the lock on CPU1 and
> the node is in list on CPU1, what will happen if these two nodes get
> deleted simultaneously?
>
> Regards,
> Boqun

Yes, you are right. I should have acquired the per-cpu head pointer 
first and used it onward instead of accessing the lock and list in 2 
separate operations. I will fix that in the next update.

Thanks for finding that.

Cheers,
Longman