Re: [Open-FCoE] [PATCH] add cpu on/offline support in Fibre Channel exchange and sequence handling

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

On Fri, 2010-10-08 at 22:57 +0800, Hillf Danton wrote:

Hi Hillf,

Can you please prefix the libfc patches with "libfc: <title>"? Also, for
your other patches that have a "SCSI" prefix, I think James' system adds
the "[SCSI]" prefix when he applies patches so you don't need to add it.

> Fibre Channel exchange pool is setup in a per cpu way that
> entire exchange id range is divided equally across all cpus.
> 
> When cpu goes offline, the corresponding range of exchg id
> becomes unavailable until it is online again.
> 
> This work tries to fix the unavailability based on notifier_block.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- o/linux-2.6.36-rc4/drivers/scsi/libfc/fc_exch.c	2010-09-13
> 07:07:38.000000000 +0800
> +++ m/linux-2.6.36-rc4/drivers/scsi/libfc/fc_exch.c	2010-10-08
> 22:24:48.000000000 +0800

Most patches I've seen on the various mailing lists are created by
diffing the files within the kernel directory, so that you don't have
the "/linux-2.6.36-rc/" in each hunk. It's the difference between using
'patch -p1' or 'patch -p2' when applying the patch.

I think this is preventing git-am from applying the patches. To import
them I ended up having git-am fail and then manually using the 'patch
-p2' command, so that I could get both the commit message from git-am
and then the patch itself from 'patch'. I don't know about anyone else,
but if you post patches without the kernel directory in the diff line it
would help me out. Thanks.

> @@ -26,6 +26,9 @@
>  #include <linux/timer.h>
>  #include <linux/slab.h>
>  #include <linux/err.h>
> +#include <linux/notifier.h>
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> 
>  #include <scsi/fc/fc_fc2.h>
> 
> @@ -40,6 +43,8 @@ static u16	fc_cpu_order;	/* 2's power to
>  static struct kmem_cache *fc_em_cachep;	       /* cache for exchanges */
>  struct workqueue_struct *fc_exch_workqueue;
> 
> +static cpumask_t fc_offline_cpu_mask = CPU_MASK_NONE;
> +
>  /*
>   * Structure and function definitions for managing Fibre Channel Exchanges
>   * and Sequences.
> @@ -666,6 +671,7 @@ static struct fc_exch *fc_exch_em_alloc(
>  	unsigned int cpu;
>  	u16 index;
>  	struct fc_exch_pool *pool;
> +	unsigned int index = -1;

This patch doesn't compile:

drivers/scsi/libfc/fc_exch.c: In function ‘fc_exch_em_alloc’:
drivers/scsi/libfc/fc_exch.c:674: error: conflicting types for ‘index’
drivers/scsi/libfc/fc_exch.c:672: note: previous declaration of ‘index’
was here
make[3]: *** [drivers/scsi/libfc/fc_exch.o] Error 1

In general I think the idea is fine. I haven't run into any
circumstances where the per-CPU pools are exhausted. I know others have
when using NPIV, but there were other ways to fix those issues.

> 
>  	/* allocate memory for exchange */
>  	ep = mempool_alloc(mp->ep_pool, GFP_ATOMIC);
> @@ -679,6 +685,7 @@ static struct fc_exch *fc_exch_em_alloc(
>  	pool = per_cpu_ptr(mp->pool, cpu);
>  	spin_lock_bh(&pool->lock);
>  	put_cpu();
> +again:
>  	index = pool->next_index;
>  	/* allocate new exch from pool */
>  	while (fc_exch_ptr_get(pool, index)) {
> @@ -719,6 +726,17 @@ out:
>  err:
>  	spin_unlock_bh(&pool->lock);
>  	atomic_inc(&mp->stats.no_free_exch_xid);

I think this should be moved below the offline pool check, when we've
really failed to allocate an exchange. It's not a per-CPU pool stat,
it's an overall resource allocation failure stat, which you're now
helping to happen less frequently. With the current placement we'll be
bumping up the counter even though we have allocated an exchange.

Thanks, //Rob

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html