Re: [PATCH 04/14] fcoe: create/destroy fcoe Rx threads on CPU hotplug events

[Robert Love <robert.w.love@intel.com>]

On Sun, 2009-03-22 at 17:59 -0700, James Bottomley wrote: 
> On Tue, 2009-03-17 at 11:41 -0700, Robert Love wrote:
> > This patch adds support for dynamically created Rx threads
> > upon CPU hotplug events.
> > 
> > There were existing synchronization problems that this patch
> > attempts to resolve. The main problem had to do with fcoe_rcv()
> > running in a different context than the hotplug notifications.
> > This opened the possiblity that fcoe_rcv() would target a Rx
> > thread for a skb. However, that thread could become NULL if
> > the CPU was made offline.
> > 
> > This patch uses the Rx queue's (a skb_queue) lock to protect
> > the thread it's associated with and we use the 'thread' member
> > of the fcoe_percpu_s to determine if the thread is ready to
> > accept new skbs.
> > 
> > The patch also attempts to do a better job of cleaning up, both
> > if hotplug registration fails as well as when the module is
> > removed.
> > 
> > Contribution provided by Joe Eykholt <jeykholt@cisco.com> to
> > fix incorrect use of __cpuinitdata.
> > 
> > Signed-off-by: Yi Zou <yi.zou@intel.com>
> > Signed-off-by: Robert Love <robert.w.love@intel.com>
> > ---
> > 
> >  drivers/scsi/fcoe/libfcoe.c |  246 +++++++++++++++++++++++++++++++++++--------
> >  1 files changed, 199 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c
> > index 648a2fc..951d244 100644
> > --- a/drivers/scsi/fcoe/libfcoe.c
> > +++ b/drivers/scsi/fcoe/libfcoe.c
> > @@ -81,6 +81,156 @@ static struct notifier_block fcoe_notifier = {
> >  };
> >  
> >  /**
> > + * fcoe_percpu_thread_create() - Create a receive thread for an online cpu
> > + * @cpu: cpu index for the online cpu
> > + */
> > +static void fcoe_percpu_thread_create(unsigned int cpu)
> > +{
> > +	struct fcoe_percpu_s *p;
> > +	struct task_struct *thread;
> > +
> > +	p = &per_cpu(fcoe_percpu, cpu);
> > +
> > +	thread = kthread_create(fcoe_percpu_receive_thread,
> > +				(void *)p, "fcoethread/%d", cpu);
> > +
> > +	if (likely(!IS_ERR(p->thread))) {
> > +		kthread_bind(thread, cpu);
> > +		wake_up_process(thread);
> > +
> > +		spin_lock_bh(&p->fcoe_rx_list.lock);
> > +		p->thread = thread;
> > +		spin_unlock_bh(&p->fcoe_rx_list.lock);
> > +	}
> > +}
> > +
> > +/**
> > + * fcoe_percpu_thread_destroy() - removes the rx thread for the given cpu
> > + * @cpu: cpu index the rx thread is to be removed
> > + *
> > + * Destroys a per-CPU Rx thread. Any pending skbs are moved to the
> > + * current CPU's Rx thread. If the thread being destroyed is bound to
> > + * the CPU processing this context the skbs will be freed.
> > + */
> > +static void fcoe_percpu_thread_destroy(unsigned int cpu)
> > +{
> > +	struct fcoe_percpu_s *p;
> > +	struct task_struct *thread;
> > +	struct page *crc_eof;
> > +	struct sk_buff *skb;
> > +#ifdef CONFIG_SMP
> > +	struct fcoe_percpu_s *p0;
> > +	unsigned targ_cpu = smp_processor_id();
> > +#endif /* CONFIG_SMP */
> > +
> > +	printk(KERN_DEBUG "fcoe: Destroying receive thread for CPU %d\n", cpu);
> > +
> > +	/* Prevent any new skbs from being queued for this CPU. */
> > +	p = &per_cpu(fcoe_percpu, cpu);
> > +	spin_lock_bh(&p->fcoe_rx_list.lock);
> > +	thread = p->thread;
> > +	p->thread = NULL;
> > +	crc_eof = p->crc_eof_page;
> > +	p->crc_eof_page = NULL;
> > +	p->crc_eof_offset = 0;
> > +	spin_unlock_bh(&p->fcoe_rx_list.lock);
> > +
> > +#ifdef CONFIG_SMP
> > +	/*
> > +	 * Don't bother moving the skb's if this context is running
> > +	 * on the same CPU that is having its thread destroyed. This
> > +	 * can easily happen when the module is removed.
> > +	 */
> > +	if (cpu != targ_cpu) {
> > +		p0 = &per_cpu(fcoe_percpu, targ_cpu);
> > +		spin_lock_bh(&p0->fcoe_rx_list.lock);
> > +		if (p0->thread) {
> > +			FC_DBG("Moving frames from CPU %d to CPU %d\n",
> > +			       cpu, targ_cpu);
> > +
> > +			while ((skb = __skb_dequeue(&p->fcoe_rx_list)) != NULL)
> > +				__skb_queue_tail(&p0->fcoe_rx_list, skb);
> > +			spin_unlock_bh(&p0->fcoe_rx_list.lock);
> > +		} else {
> > +			/*
> > +			 * The targeted CPU is not initialized and cannot accept
> > +			 * new  skbs. Unlock the targeted CPU and drop the skbs
> > +			 * on the CPU that is going offline.
> > +			 */
> > +			while ((skb = __skb_dequeue(&p->fcoe_rx_list)) != NULL)
> > +				kfree_skb(skb);
> > +			spin_unlock_bh(&p0->fcoe_rx_list.lock);
> > +		}
> > +	} else {
> > +		/*
> > +		 * This scenario occurs when the module is being removed
> > +		 * and all threads are being destroyed. skbs will continue
> > +		 * to be shifted from the CPU thread that is being removed
> > +		 * to the CPU thread associated with the CPU that is processing
> > +		 * the module removal. Once there is only one CPU Rx thread it
> > +		 * will reach this case and we will drop all skbs and later
> > +		 * stop the thread.
> > +		 */
> > +		spin_lock_bh(&p->fcoe_rx_list.lock);
> > +		while ((skb = __skb_dequeue(&p->fcoe_rx_list)) != NULL)
> > +			kfree_skb(skb);
> > +		spin_unlock_bh(&p->fcoe_rx_list.lock);
> > +	}
> > +#else
> > +	/*
> > +	 * This a non-SMP scenario where the singluar Rx thread is
> > +	 * being removed. Free all skbs and stop the thread.
> > +	 */
> > +	spin_lock_bh(&p->fcoe_rx_list.lock);
> > +	while ((skb = __skb_dequeue(&p->fcoe_rx_list)) != NULL)
> > +		kfree_skb(skb);
> > +	spin_unlock_bh(&p->fcoe_rx_list.lock);
> > +#endif
> > +
> > +	if (thread)
> > +		kthread_stop(thread);
> 
> Don't you need a kthread_unbind() somewhere in here?  Under most of your
> calling conditions (CPU_DEAD events) the bound CPU is toast, so the
> thread isn't going to be able to wake up on it.
> 
Is there a kthread_unbind()? I can't find it, maybe it was removed
recently.

This function should be moving all work off of the CPU that's going away
and should also prevent any new work from being scheduled to it (by
setting p->thread = NULL), so I'm not expecting this thread to wake up
and do something.