* [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
@ 2012-03-09 22:49 ` Robert Love
2012-03-09 23:50 ` Bhanu Prakash Gollapudi
2012-03-09 22:49 ` [PATCH 02/10] fcoe: Ensure fcoe_recv_frame is always called in process context Robert Love
` (8 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:49 UTC (permalink / raw)
To: linux-scsi; +Cc: Neil Horman
From: Neil Horman <nhorman@tuxdriver.com>
Recieved this warning recently:
WARNING: at kernel/softirq.c:143 local_bh_enable+0x7d/0xb0() (Not tainted)
Hardware name: C6100
Modules linked in: autofs4 bnx2fc cnic uio fcoe libfcoe libfc scsi_transport_fc
scsi_tgt 8021q garp stp llc sunrpc cpufreq_ondemand acpi_cpufreq freq_table
mperf ipv6 ixgbe mdio igb dcdbas microcode i2c_i801 i2c_core sg iTCO_wdt
iTCO_vendor_support ioatdma dca i7core_edac edac_core shpchp ext4 mbcache jbd2
sd_mod crc_t10dif ahci dm_mirror dm_region_hash dm_log dm_mod [last unloaded:
scsi_wait_scan]
Pid: 4926, comm: fc_rport_eq Not tainted 2.6.32 #2
Call Trace:
[<ffffffff81069c67>] ? warn_slowpath_common+0x87/0xc0
[<ffffffff81069cba>] ? warn_slowpath_null+0x1a/0x20
[<ffffffff8107216d>] ? local_bh_enable+0x7d/0xb0
[<ffffffff81433567>] ? dev_queue_xmit+0x177/0x6c0
[<ffffffffa0293a34>] ? vlan_dev_hwaccel_hard_start_xmit+0x84/0xb0 [8021q]
[<ffffffff8142f46c>] ? dev_hard_start_xmit+0x2bc/0x3f0
[<ffffffff8143392e>] ? dev_queue_xmit+0x53e/0x6c0
[<ffffffff8142348e>] ? __skb_clone+0x2e/0x120
[<ffffffffa02ea83d>] ? fcoe_clean_pending_queue+0xcd/0x100 [libfcoe]
[<ffffffff810559ca>] ? find_busiest_group+0x9aa/0xb20
[<ffffffffa02f7624>] ? fcoe_port_send+0x24/0x50 [fcoe]
[<ffffffffa02f9568>] ? fcoe_xmit+0x228/0x3e0 [fcoe]
[<ffffffffa02c1896>] ? fc_seq_send+0xb6/0x160 [libfc]
[<ffffffffa02c1a76>] ? fc_exch_abort_locked+0x136/0x1c0 [libfc]
[<ffffffffa02c1ca7>] ? fc_exch_mgr_reset+0x147/0x2a0 [libfc]
[<ffffffff814f2c7e>] ? _spin_unlock_irq+0xe/0x20
[<ffffffffa02c91d3>] ? fc_rport_work+0x123/0x5f0 [libfc]
[<ffffffffa02c90b0>] ? fc_rport_work+0x0/0x5f0 [libfc]
[<ffffffff8108b559>] ? worker_thread+0x179/0x2a0
[<ffffffff81090ea0>] ? autoremove_wake_function+0x0/0x40
[<ffffffff8108b3e0>] ? worker_thread+0x0/0x2a0
[<ffffffff81090b56>] ? kthread+0x96/0xa0
[<ffffffff8100c10a>] ? child_rip+0xa/0x20
[<ffffffff81090ac0>] ? kthread+0x0/0xa0
[<ffffffff8100c100>] ? child_rip+0x0/0x20
fc_exch_abort_locked attempts to send an abort frame, but it does so with a
spinlock held and irqs/bh's disabled. Because of this dev_queue_xmit issues a
warning. This patch allows the fc_exch_abort_locked to drop and re-acquire the
lock so that the warning is suppressed.
Note this isn't a great fix, given that we need to free and re-acquire the
ex_lock during the fc_exch_abort_locked operation, but any other solution is
going to be a fairly major undertaking I think. This should work until a proper
fix can be found.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/libfc/fc_exch.c | 31 ++++++++++++++++++++++++++-----
include/scsi/libfc.h | 1 +
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 630291f..588060f 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -594,16 +594,23 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
struct fc_frame *fp;
int error;
+ if (ep->in_locked_transaction == true)
+ return -EAGAIN;
+
+ ep->in_locked_transaction = true;
+
+ error = -ENXIO;
if (ep->esb_stat & (ESB_ST_COMPLETE | ESB_ST_ABNORMAL) ||
ep->state & (FC_EX_DONE | FC_EX_RST_CLEANUP))
- return -ENXIO;
+ goto out;
/*
* Send the abort on a new sequence if possible.
*/
+ error = -ENOMEM;
sp = fc_seq_start_next_locked(&ep->seq);
if (!sp)
- return -ENOMEM;
+ goto out;
ep->esb_stat |= ESB_ST_SEQ_INIT | ESB_ST_ABNORMAL;
if (timer_msec)
@@ -613,8 +620,9 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
* If not logged into the fabric, don't send ABTS but leave
* sequence active until next timeout.
*/
+ error = 0;
if (!ep->sid)
- return 0;
+ goto out;
/*
* Send an abort for the sequence that timed out.
@@ -623,9 +631,13 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
if (fp) {
fc_fill_fc_hdr(fp, FC_RCTL_BA_ABTS, ep->did, ep->sid,
FC_TYPE_BLS, FC_FC_END_SEQ | FC_FC_SEQ_INIT, 0);
+ spin_unlock_bh(&ep->ex_lock);
error = fc_seq_send(ep->lp, sp, fp);
+ spin_lock_bh(&ep->ex_lock);
} else
error = -ENOBUFS;
+out:
+ ep->in_locked_transaction = false;
return error;
}
@@ -645,9 +657,12 @@ static int fc_seq_exch_abort(const struct fc_seq *req_sp,
int error;
ep = fc_seq_exch(req_sp);
+retry:
spin_lock_bh(&ep->ex_lock);
error = fc_exch_abort_locked(ep, timer_msec);
spin_unlock_bh(&ep->ex_lock);
+ if (error == -EAGAIN)
+ goto retry;
return error;
}
@@ -1732,10 +1747,16 @@ static void fc_exch_reset(struct fc_exch *ep)
struct fc_seq *sp;
void (*resp)(struct fc_seq *, struct fc_frame *, void *);
void *arg;
- int rc = 1;
+ int rc;
+retry:
spin_lock_bh(&ep->ex_lock);
- fc_exch_abort_locked(ep, 0);
+ rc = fc_exch_abort_locked(ep, 0);
+ if (rc == -EAGAIN) {
+ spin_unlock_bh(&ep->ex_lock);
+ goto retry;
+ }
+
ep->state |= FC_EX_RST_CLEANUP;
if (cancel_delayed_work(&ep->timeout_work))
atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
index 8f9dfba..0a4ceb8 100644
--- a/include/scsi/libfc.h
+++ b/include/scsi/libfc.h
@@ -438,6 +438,7 @@ struct fc_exch {
void (*resp)(struct fc_seq *, struct fc_frame *, void *);
void *arg;
void (*destructor)(struct fc_seq *, void *);
+ bool in_locked_transaction;
struct delayed_work timeout_work;
} ____cacheline_aligned_in_smp;
#define fc_seq_exch(sp) container_of(sp, struct fc_exch, seq)
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled
2012-03-09 22:49 ` [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled Robert Love
@ 2012-03-09 23:50 ` Bhanu Prakash Gollapudi
2012-03-10 0:12 ` Love, Robert W
[not found] ` <4F5A9754.6080009-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
0 siblings, 2 replies; 17+ messages in thread
From: Bhanu Prakash Gollapudi @ 2012-03-09 23:50 UTC (permalink / raw)
To: Robert Love; +Cc: linux-scsi@vger.kernel.org, Neil Horman, devel@open-fcoe.org
On 3/9/2012 2:49 PM, Robert Love wrote:
> From: Neil Horman<nhorman@tuxdriver.com>
>
> Recieved this warning recently:
>
> WARNING: at kernel/softirq.c:143 local_bh_enable+0x7d/0xb0() (Not tainted)
> Hardware name: C6100
> Modules linked in: autofs4 bnx2fc cnic uio fcoe libfcoe libfc scsi_transport_fc
> scsi_tgt 8021q garp stp llc sunrpc cpufreq_ondemand acpi_cpufreq freq_table
> mperf ipv6 ixgbe mdio igb dcdbas microcode i2c_i801 i2c_core sg iTCO_wdt
> iTCO_vendor_support ioatdma dca i7core_edac edac_core shpchp ext4 mbcache jbd2
> sd_mod crc_t10dif ahci dm_mirror dm_region_hash dm_log dm_mod [last unloaded:
> scsi_wait_scan]
> Pid: 4926, comm: fc_rport_eq Not tainted 2.6.32 #2
> Call Trace:
> [<ffffffff81069c67>] ? warn_slowpath_common+0x87/0xc0
> [<ffffffff81069cba>] ? warn_slowpath_null+0x1a/0x20
> [<ffffffff8107216d>] ? local_bh_enable+0x7d/0xb0
> [<ffffffff81433567>] ? dev_queue_xmit+0x177/0x6c0
> [<ffffffffa0293a34>] ? vlan_dev_hwaccel_hard_start_xmit+0x84/0xb0 [8021q]
> [<ffffffff8142f46c>] ? dev_hard_start_xmit+0x2bc/0x3f0
> [<ffffffff8143392e>] ? dev_queue_xmit+0x53e/0x6c0
> [<ffffffff8142348e>] ? __skb_clone+0x2e/0x120
> [<ffffffffa02ea83d>] ? fcoe_clean_pending_queue+0xcd/0x100 [libfcoe]
> [<ffffffff810559ca>] ? find_busiest_group+0x9aa/0xb20
> [<ffffffffa02f7624>] ? fcoe_port_send+0x24/0x50 [fcoe]
> [<ffffffffa02f9568>] ? fcoe_xmit+0x228/0x3e0 [fcoe]
> [<ffffffffa02c1896>] ? fc_seq_send+0xb6/0x160 [libfc]
> [<ffffffffa02c1a76>] ? fc_exch_abort_locked+0x136/0x1c0 [libfc]
> [<ffffffffa02c1ca7>] ? fc_exch_mgr_reset+0x147/0x2a0 [libfc]
> [<ffffffff814f2c7e>] ? _spin_unlock_irq+0xe/0x20
> [<ffffffffa02c91d3>] ? fc_rport_work+0x123/0x5f0 [libfc]
> [<ffffffffa02c90b0>] ? fc_rport_work+0x0/0x5f0 [libfc]
> [<ffffffff8108b559>] ? worker_thread+0x179/0x2a0
> [<ffffffff81090ea0>] ? autoremove_wake_function+0x0/0x40
> [<ffffffff8108b3e0>] ? worker_thread+0x0/0x2a0
> [<ffffffff81090b56>] ? kthread+0x96/0xa0
> [<ffffffff8100c10a>] ? child_rip+0xa/0x20
> [<ffffffff81090ac0>] ? kthread+0x0/0xa0
> [<ffffffff8100c100>] ? child_rip+0x0/0x20
>
> fc_exch_abort_locked attempts to send an abort frame, but it does so with a
> spinlock held and irqs/bh's disabled. Because of this dev_queue_xmit issues a
> warning. This patch allows the fc_exch_abort_locked to drop and re-acquire the
> lock so that the warning is suppressed.
>
> Note this isn't a great fix, given that we need to free and re-acquire the
> ex_lock during the fc_exch_abort_locked operation, but any other solution is
> going to be a fairly major undertaking I think. This should work until a proper
> fix can be found.
>
> Signed-off-by: Neil Horman<nhorman@tuxdriver.com>
> Signed-off-by: Robert Love<robert.w.love@intel.com>
Robert, This patch is not required based on what we discussed in this
thread..
http://lists.open-fcoe.org/pipermail/devel/2012-February/011946.html
Neil provided "fcoe: Ensure fcoe_recv_frame is always called in process
context" instead of the two patches he submitted initially.
http://lists.open-fcoe.org/pipermail/devel/2012-February/011962.html
Neil, correct me if I'm wrong.
Thanks,
Bhanu
> ---
> drivers/scsi/libfc/fc_exch.c | 31 ++++++++++++++++++++++++++-----
> include/scsi/libfc.h | 1 +
> 2 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
> index 630291f..588060f 100644
> --- a/drivers/scsi/libfc/fc_exch.c
> +++ b/drivers/scsi/libfc/fc_exch.c
> @@ -594,16 +594,23 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
> struct fc_frame *fp;
> int error;
>
> + if (ep->in_locked_transaction == true)
> + return -EAGAIN;
> +
> + ep->in_locked_transaction = true;
> +
> + error = -ENXIO;
> if (ep->esb_stat& (ESB_ST_COMPLETE | ESB_ST_ABNORMAL) ||
> ep->state& (FC_EX_DONE | FC_EX_RST_CLEANUP))
> - return -ENXIO;
> + goto out;
>
> /*
> * Send the abort on a new sequence if possible.
> */
> + error = -ENOMEM;
> sp = fc_seq_start_next_locked(&ep->seq);
> if (!sp)
> - return -ENOMEM;
> + goto out;
>
> ep->esb_stat |= ESB_ST_SEQ_INIT | ESB_ST_ABNORMAL;
> if (timer_msec)
> @@ -613,8 +620,9 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
> * If not logged into the fabric, don't send ABTS but leave
> * sequence active until next timeout.
> */
> + error = 0;
> if (!ep->sid)
> - return 0;
> + goto out;
>
> /*
> * Send an abort for the sequence that timed out.
> @@ -623,9 +631,13 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
> if (fp) {
> fc_fill_fc_hdr(fp, FC_RCTL_BA_ABTS, ep->did, ep->sid,
> FC_TYPE_BLS, FC_FC_END_SEQ | FC_FC_SEQ_INIT, 0);
> + spin_unlock_bh(&ep->ex_lock);
> error = fc_seq_send(ep->lp, sp, fp);
> + spin_lock_bh(&ep->ex_lock);
> } else
> error = -ENOBUFS;
> +out:
> + ep->in_locked_transaction = false;
> return error;
> }
>
> @@ -645,9 +657,12 @@ static int fc_seq_exch_abort(const struct fc_seq *req_sp,
> int error;
>
> ep = fc_seq_exch(req_sp);
> +retry:
> spin_lock_bh(&ep->ex_lock);
> error = fc_exch_abort_locked(ep, timer_msec);
> spin_unlock_bh(&ep->ex_lock);
> + if (error == -EAGAIN)
> + goto retry;
> return error;
> }
>
> @@ -1732,10 +1747,16 @@ static void fc_exch_reset(struct fc_exch *ep)
> struct fc_seq *sp;
> void (*resp)(struct fc_seq *, struct fc_frame *, void *);
> void *arg;
> - int rc = 1;
> + int rc;
>
> +retry:
> spin_lock_bh(&ep->ex_lock);
> - fc_exch_abort_locked(ep, 0);
> + rc = fc_exch_abort_locked(ep, 0);
> + if (rc == -EAGAIN) {
> + spin_unlock_bh(&ep->ex_lock);
> + goto retry;
> + }
> +
> ep->state |= FC_EX_RST_CLEANUP;
> if (cancel_delayed_work(&ep->timeout_work))
> atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
> diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
> index 8f9dfba..0a4ceb8 100644
> --- a/include/scsi/libfc.h
> +++ b/include/scsi/libfc.h
> @@ -438,6 +438,7 @@ struct fc_exch {
> void (*resp)(struct fc_seq *, struct fc_frame *, void *);
> void *arg;
> void (*destructor)(struct fc_seq *, void *);
> + bool in_locked_transaction;
> struct delayed_work timeout_work;
> } ____cacheline_aligned_in_smp;
> #define fc_seq_exch(sp) container_of(sp, struct fc_exch, seq)
>
> --
> 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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled
2012-03-09 23:50 ` Bhanu Prakash Gollapudi
@ 2012-03-10 0:12 ` Love, Robert W
[not found] ` <4F5A9C53.5040200-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
[not found] ` <4F5A9754.6080009-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
1 sibling, 1 reply; 17+ messages in thread
From: Love, Robert W @ 2012-03-10 0:12 UTC (permalink / raw)
To: Bhanu Prakash Gollapudi
Cc: linux-scsi@vger.kernel.org, Neil Horman, devel@open-fcoe.org
On 03/09/2012 03:50 PM, Bhanu Prakash Gollapudi wrote:
> On 3/9/2012 2:49 PM, Robert Love wrote:
>> From: Neil Horman<nhorman@tuxdriver.com>
>>
>> Recieved this warning recently:
>>
>> WARNING: at kernel/softirq.c:143 local_bh_enable+0x7d/0xb0() (Not
>> tainted)
>> Hardware name: C6100
>> Modules linked in: autofs4 bnx2fc cnic uio fcoe libfcoe libfc
>> scsi_transport_fc
>> scsi_tgt 8021q garp stp llc sunrpc cpufreq_ondemand acpi_cpufreq
>> freq_table
>> mperf ipv6 ixgbe mdio igb dcdbas microcode i2c_i801 i2c_core sg iTCO_wdt
>> iTCO_vendor_support ioatdma dca i7core_edac edac_core shpchp ext4
>> mbcache jbd2
>> sd_mod crc_t10dif ahci dm_mirror dm_region_hash dm_log dm_mod [last
>> unloaded:
>> scsi_wait_scan]
>> Pid: 4926, comm: fc_rport_eq Not tainted 2.6.32 #2
>> Call Trace:
>> [<ffffffff81069c67>] ? warn_slowpath_common+0x87/0xc0
>> [<ffffffff81069cba>] ? warn_slowpath_null+0x1a/0x20
>> [<ffffffff8107216d>] ? local_bh_enable+0x7d/0xb0
>> [<ffffffff81433567>] ? dev_queue_xmit+0x177/0x6c0
>> [<ffffffffa0293a34>] ? vlan_dev_hwaccel_hard_start_xmit+0x84/0xb0
>> [8021q]
>> [<ffffffff8142f46c>] ? dev_hard_start_xmit+0x2bc/0x3f0
>> [<ffffffff8143392e>] ? dev_queue_xmit+0x53e/0x6c0
>> [<ffffffff8142348e>] ? __skb_clone+0x2e/0x120
>> [<ffffffffa02ea83d>] ? fcoe_clean_pending_queue+0xcd/0x100 [libfcoe]
>> [<ffffffff810559ca>] ? find_busiest_group+0x9aa/0xb20
>> [<ffffffffa02f7624>] ? fcoe_port_send+0x24/0x50 [fcoe]
>> [<ffffffffa02f9568>] ? fcoe_xmit+0x228/0x3e0 [fcoe]
>> [<ffffffffa02c1896>] ? fc_seq_send+0xb6/0x160 [libfc]
>> [<ffffffffa02c1a76>] ? fc_exch_abort_locked+0x136/0x1c0 [libfc]
>> [<ffffffffa02c1ca7>] ? fc_exch_mgr_reset+0x147/0x2a0 [libfc]
>> [<ffffffff814f2c7e>] ? _spin_unlock_irq+0xe/0x20
>> [<ffffffffa02c91d3>] ? fc_rport_work+0x123/0x5f0 [libfc]
>> [<ffffffffa02c90b0>] ? fc_rport_work+0x0/0x5f0 [libfc]
>> [<ffffffff8108b559>] ? worker_thread+0x179/0x2a0
>> [<ffffffff81090ea0>] ? autoremove_wake_function+0x0/0x40
>> [<ffffffff8108b3e0>] ? worker_thread+0x0/0x2a0
>> [<ffffffff81090b56>] ? kthread+0x96/0xa0
>> [<ffffffff8100c10a>] ? child_rip+0xa/0x20
>> [<ffffffff81090ac0>] ? kthread+0x0/0xa0
>> [<ffffffff8100c100>] ? child_rip+0x0/0x20
>>
>> fc_exch_abort_locked attempts to send an abort frame, but it does so
>> with a
>> spinlock held and irqs/bh's disabled. Because of this dev_queue_xmit
>> issues a
>> warning. This patch allows the fc_exch_abort_locked to drop and
>> re-acquire the
>> lock so that the warning is suppressed.
>>
>> Note this isn't a great fix, given that we need to free and
>> re-acquire the
>> ex_lock during the fc_exch_abort_locked operation, but any other
>> solution is
>> going to be a fairly major undertaking I think. This should work
>> until a proper
>> fix can be found.
>>
>> Signed-off-by: Neil Horman<nhorman@tuxdriver.com>
>> Signed-off-by: Robert Love<robert.w.love@intel.com>
>
> Robert, This patch is not required based on what we discussed in this
> thread..
> http://lists.open-fcoe.org/pipermail/devel/2012-February/011946.html
>
> Neil provided "fcoe: Ensure fcoe_recv_frame is always called in
> process context" instead of the two patches he submitted initially.
> http://lists.open-fcoe.org/pipermail/devel/2012-February/011962.html
>
Thanks for pointing this out Bhanu, I overlooked the last comment in the
discussion of this patch.
James, I believe the correct patch is 02/10 in this series, so I think
you should be able to drop this patch and we'll be back on track. If
you'd like me to resend the series without this patch please let me
know. I'll check to make sure 02/10 through 10/10 apply without 01/10
and resend if I have to make changes.
For formality's sake:
Nacked-by: Robert Love <robert.w.love@intel.com>
Thanks, //Rob
^ permalink raw reply [flat|nested] 17+ messages in thread[parent not found: <4F5A9754.6080009-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled
[not found] ` <4F5A9754.6080009-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
@ 2012-03-11 17:43 ` Neil Horman
2012-03-12 3:25 ` Bhanu Prakash Gollapudi
0 siblings, 1 reply; 17+ messages in thread
From: Neil Horman @ 2012-03-11 17:43 UTC (permalink / raw)
To: Bhanu Prakash Gollapudi
Cc: devel-s9riP+hp16TNLxjTenLetw@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Fri, Mar 09, 2012 at 03:50:44PM -0800, Bhanu Prakash Gollapudi wrote:
> On 3/9/2012 2:49 PM, Robert Love wrote:
> >From: Neil Horman<nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> >
> >Recieved this warning recently:
> >
> >WARNING: at kernel/softirq.c:143 local_bh_enable+0x7d/0xb0() (Not tainted)
> >Hardware name: C6100
> >Modules linked in: autofs4 bnx2fc cnic uio fcoe libfcoe libfc scsi_transport_fc
> >scsi_tgt 8021q garp stp llc sunrpc cpufreq_ondemand acpi_cpufreq freq_table
> >mperf ipv6 ixgbe mdio igb dcdbas microcode i2c_i801 i2c_core sg iTCO_wdt
> >iTCO_vendor_support ioatdma dca i7core_edac edac_core shpchp ext4 mbcache jbd2
> >sd_mod crc_t10dif ahci dm_mirror dm_region_hash dm_log dm_mod [last unloaded:
> >scsi_wait_scan]
> >Pid: 4926, comm: fc_rport_eq Not tainted 2.6.32 #2
> >Call Trace:
> > [<ffffffff81069c67>] ? warn_slowpath_common+0x87/0xc0
> > [<ffffffff81069cba>] ? warn_slowpath_null+0x1a/0x20
> > [<ffffffff8107216d>] ? local_bh_enable+0x7d/0xb0
> > [<ffffffff81433567>] ? dev_queue_xmit+0x177/0x6c0
> > [<ffffffffa0293a34>] ? vlan_dev_hwaccel_hard_start_xmit+0x84/0xb0 [8021q]
> > [<ffffffff8142f46c>] ? dev_hard_start_xmit+0x2bc/0x3f0
> > [<ffffffff8143392e>] ? dev_queue_xmit+0x53e/0x6c0
> > [<ffffffff8142348e>] ? __skb_clone+0x2e/0x120
> > [<ffffffffa02ea83d>] ? fcoe_clean_pending_queue+0xcd/0x100 [libfcoe]
> > [<ffffffff810559ca>] ? find_busiest_group+0x9aa/0xb20
> > [<ffffffffa02f7624>] ? fcoe_port_send+0x24/0x50 [fcoe]
> > [<ffffffffa02f9568>] ? fcoe_xmit+0x228/0x3e0 [fcoe]
> > [<ffffffffa02c1896>] ? fc_seq_send+0xb6/0x160 [libfc]
> > [<ffffffffa02c1a76>] ? fc_exch_abort_locked+0x136/0x1c0 [libfc]
> > [<ffffffffa02c1ca7>] ? fc_exch_mgr_reset+0x147/0x2a0 [libfc]
> > [<ffffffff814f2c7e>] ? _spin_unlock_irq+0xe/0x20
> > [<ffffffffa02c91d3>] ? fc_rport_work+0x123/0x5f0 [libfc]
> > [<ffffffffa02c90b0>] ? fc_rport_work+0x0/0x5f0 [libfc]
> > [<ffffffff8108b559>] ? worker_thread+0x179/0x2a0
> > [<ffffffff81090ea0>] ? autoremove_wake_function+0x0/0x40
> > [<ffffffff8108b3e0>] ? worker_thread+0x0/0x2a0
> > [<ffffffff81090b56>] ? kthread+0x96/0xa0
> > [<ffffffff8100c10a>] ? child_rip+0xa/0x20
> > [<ffffffff81090ac0>] ? kthread+0x0/0xa0
> > [<ffffffff8100c100>] ? child_rip+0x0/0x20
> >
> >fc_exch_abort_locked attempts to send an abort frame, but it does so with a
> >spinlock held and irqs/bh's disabled. Because of this dev_queue_xmit issues a
> >warning. This patch allows the fc_exch_abort_locked to drop and re-acquire the
> >lock so that the warning is suppressed.
> >
> >Note this isn't a great fix, given that we need to free and re-acquire the
> >ex_lock during the fc_exch_abort_locked operation, but any other solution is
> >going to be a fairly major undertaking I think. This should work until a proper
> >fix can be found.
> >
> >Signed-off-by: Neil Horman<nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> >Signed-off-by: Robert Love<robert.w.love-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> Robert, This patch is not required based on what we discussed in
> this thread..
> http://lists.open-fcoe.org/pipermail/devel/2012-February/011946.html
>
> Neil provided "fcoe: Ensure fcoe_recv_frame is always called in
> process context" instead of the two patches he submitted initially.
> http://lists.open-fcoe.org/pipermail/devel/2012-February/011962.html
>
> Neil, correct me if I'm wrong.
>
Bhanu, This is not quite right (although that was a very confusing thread). The
ema_list fix that I was proposing is no longer necessecary, because after that
conversation we realized the whole issue cold be avoided by making sure that we
never allowed fcoe_recv_frame to be called in process context.
This patch is still necessecary. Its needed beacuse fc_exch_abort_locked is (by
definition) called with the ex_lock held. fc_exch_abort_locked however calls
fc_seq_send, which can lock the same ex_lock, causing deadlock.
So yes, we need a fix for this. That said, I'd just as soon you drop this
patch, as I was looking at this code and realized that I can make a much more
efficient fix for this problem. So let me know how you want to handle this: If
you want to drop this patch, I'll have an improved fix for you early this week.
If you want to keep this patch in place, thats fine with me too. My new fix
will just revert these bits. Let me know how you want to play it.
Thank you for bringing this up though, I'd been meaning to review this and see
if I could fix it a bit better.
Best
Neil
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled
2012-03-11 17:43 ` Neil Horman
@ 2012-03-12 3:25 ` Bhanu Prakash Gollapudi
2012-03-12 10:42 ` Neil Horman
0 siblings, 1 reply; 17+ messages in thread
From: Bhanu Prakash Gollapudi @ 2012-03-12 3:25 UTC (permalink / raw)
To: Neil Horman; +Cc: Robert Love, linux-scsi@vger.kernel.org, devel@open-fcoe.org
On 3/11/2012 10:43 AM, Neil Horman wrote:
> On Fri, Mar 09, 2012 at 03:50:44PM -0800, Bhanu Prakash Gollapudi wrote:
>> On 3/9/2012 2:49 PM, Robert Love wrote:
>>> From: Neil Horman<nhorman@tuxdriver.com>
>>>
>>> Recieved this warning recently:
>>>
>>> WARNING: at kernel/softirq.c:143 local_bh_enable+0x7d/0xb0() (Not tainted)
>>> Hardware name: C6100
>>> Modules linked in: autofs4 bnx2fc cnic uio fcoe libfcoe libfc scsi_transport_fc
>>> scsi_tgt 8021q garp stp llc sunrpc cpufreq_ondemand acpi_cpufreq freq_table
>>> mperf ipv6 ixgbe mdio igb dcdbas microcode i2c_i801 i2c_core sg iTCO_wdt
>>> iTCO_vendor_support ioatdma dca i7core_edac edac_core shpchp ext4 mbcache jbd2
>>> sd_mod crc_t10dif ahci dm_mirror dm_region_hash dm_log dm_mod [last unloaded:
>>> scsi_wait_scan]
>>> Pid: 4926, comm: fc_rport_eq Not tainted 2.6.32 #2
>>> Call Trace:
>>> [<ffffffff81069c67>] ? warn_slowpath_common+0x87/0xc0
>>> [<ffffffff81069cba>] ? warn_slowpath_null+0x1a/0x20
>>> [<ffffffff8107216d>] ? local_bh_enable+0x7d/0xb0
>>> [<ffffffff81433567>] ? dev_queue_xmit+0x177/0x6c0
>>> [<ffffffffa0293a34>] ? vlan_dev_hwaccel_hard_start_xmit+0x84/0xb0 [8021q]
>>> [<ffffffff8142f46c>] ? dev_hard_start_xmit+0x2bc/0x3f0
>>> [<ffffffff8143392e>] ? dev_queue_xmit+0x53e/0x6c0
>>> [<ffffffff8142348e>] ? __skb_clone+0x2e/0x120
>>> [<ffffffffa02ea83d>] ? fcoe_clean_pending_queue+0xcd/0x100 [libfcoe]
>>> [<ffffffff810559ca>] ? find_busiest_group+0x9aa/0xb20
>>> [<ffffffffa02f7624>] ? fcoe_port_send+0x24/0x50 [fcoe]
>>> [<ffffffffa02f9568>] ? fcoe_xmit+0x228/0x3e0 [fcoe]
>>> [<ffffffffa02c1896>] ? fc_seq_send+0xb6/0x160 [libfc]
>>> [<ffffffffa02c1a76>] ? fc_exch_abort_locked+0x136/0x1c0 [libfc]
>>> [<ffffffffa02c1ca7>] ? fc_exch_mgr_reset+0x147/0x2a0 [libfc]
>>> [<ffffffff814f2c7e>] ? _spin_unlock_irq+0xe/0x20
>>> [<ffffffffa02c91d3>] ? fc_rport_work+0x123/0x5f0 [libfc]
>>> [<ffffffffa02c90b0>] ? fc_rport_work+0x0/0x5f0 [libfc]
>>> [<ffffffff8108b559>] ? worker_thread+0x179/0x2a0
>>> [<ffffffff81090ea0>] ? autoremove_wake_function+0x0/0x40
>>> [<ffffffff8108b3e0>] ? worker_thread+0x0/0x2a0
>>> [<ffffffff81090b56>] ? kthread+0x96/0xa0
>>> [<ffffffff8100c10a>] ? child_rip+0xa/0x20
>>> [<ffffffff81090ac0>] ? kthread+0x0/0xa0
>>> [<ffffffff8100c100>] ? child_rip+0x0/0x20
>>>
>>> fc_exch_abort_locked attempts to send an abort frame, but it does so with a
>>> spinlock held and irqs/bh's disabled. Because of this dev_queue_xmit issues a
>>> warning. This patch allows the fc_exch_abort_locked to drop and re-acquire the
>>> lock so that the warning is suppressed.
>>>
>>> Note this isn't a great fix, given that we need to free and re-acquire the
>>> ex_lock during the fc_exch_abort_locked operation, but any other solution is
>>> going to be a fairly major undertaking I think. This should work until a proper
>>> fix can be found.
>>>
>>> Signed-off-by: Neil Horman<nhorman@tuxdriver.com>
>>> Signed-off-by: Robert Love<robert.w.love@intel.com>
>>
>> Robert, This patch is not required based on what we discussed in
>> this thread..
>> http://lists.open-fcoe.org/pipermail/devel/2012-February/011946.html
>>
>> Neil provided "fcoe: Ensure fcoe_recv_frame is always called in
>> process context" instead of the two patches he submitted initially.
>> http://lists.open-fcoe.org/pipermail/devel/2012-February/011962.html
>>
>> Neil, correct me if I'm wrong.
>>
> Bhanu, This is not quite right (although that was a very confusing thread). The
> ema_list fix that I was proposing is no longer necessecary, because after that
> conversation we realized the whole issue cold be avoided by making sure that we
> never allowed fcoe_recv_frame to be called in process context.
>
> This patch is still necessecary.
Neil, The reason you saw this (WARNING) stack trace (in softirq.c) was
because of using spin_lock_irqsave() instead of spin_lock_bh() in
fc_exch_reset() as part of "[PATCH 1/2] fcoe: provide exclusion to
concurrent ema_list access", which was not eventually submitted.
Because of using spin_lock_irqsave(), the following WARN_ON is hit in
_local_bh_enable_ip().
static inline void _local_bh_enable_ip(unsigned long ip)
{
WARN_ON_ONCE(in_irq() || irqs_disabled());
I don't think you will see this WARNING with the existing code.
Its needed beacuse fc_exch_abort_locked is (by
> definition) called with the ex_lock held. fc_exch_abort_locked however calls
> fc_seq_send, which can lock the same ex_lock, causing deadlock.
This is NOT an issue. Although fc_seq_send takes ex_lock,
fc_exch_abort_locked() code path does not come here, as we return from
this function if fh_type is BLS.
if (fh_type == FC_TYPE_BLS)
return error;
/*
* Update the exchange and sequence flags,
* assuming all frames for the sequence have been sent.
* We can only be called to send once for each sequence.
*/
spin_lock_bh(&ep->ex_lock);
...
Thanks,
Bhanu
>
> So yes, we need a fix for this. That said, I'd just as soon you drop this
> patch, as I was looking at this code and realized that I can make a much more
> efficient fix for this problem. So let me know how you want to handle this: If
> you want to drop this patch, I'll have an improved fix for you early this week.
> If you want to keep this patch in place, thats fine with me too. My new fix
> will just revert these bits. Let me know how you want to play it.
>
> Thank you for bringing this up though, I'd been meaning to review this and see
> if I could fix it a bit better.
>
> Best
> Neil
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled
2012-03-12 3:25 ` Bhanu Prakash Gollapudi
@ 2012-03-12 10:42 ` Neil Horman
0 siblings, 0 replies; 17+ messages in thread
From: Neil Horman @ 2012-03-12 10:42 UTC (permalink / raw)
To: Bhanu Prakash Gollapudi
Cc: Robert Love, linux-scsi@vger.kernel.org, devel@open-fcoe.org
On Sun, Mar 11, 2012 at 08:25:30PM -0700, Bhanu Prakash Gollapudi wrote:
> On 3/11/2012 10:43 AM, Neil Horman wrote:
> >On Fri, Mar 09, 2012 at 03:50:44PM -0800, Bhanu Prakash Gollapudi wrote:
> >>On 3/9/2012 2:49 PM, Robert Love wrote:
> >>>From: Neil Horman<nhorman@tuxdriver.com>
> >>>
> >>>Recieved this warning recently:
> >>>
> >>>WARNING: at kernel/softirq.c:143 local_bh_enable+0x7d/0xb0() (Not tainted)
> >>>Hardware name: C6100
> >>>Modules linked in: autofs4 bnx2fc cnic uio fcoe libfcoe libfc scsi_transport_fc
> >>>scsi_tgt 8021q garp stp llc sunrpc cpufreq_ondemand acpi_cpufreq freq_table
> >>>mperf ipv6 ixgbe mdio igb dcdbas microcode i2c_i801 i2c_core sg iTCO_wdt
> >>>iTCO_vendor_support ioatdma dca i7core_edac edac_core shpchp ext4 mbcache jbd2
> >>>sd_mod crc_t10dif ahci dm_mirror dm_region_hash dm_log dm_mod [last unloaded:
> >>>scsi_wait_scan]
> >>>Pid: 4926, comm: fc_rport_eq Not tainted 2.6.32 #2
> >>>Call Trace:
> >>> [<ffffffff81069c67>] ? warn_slowpath_common+0x87/0xc0
> >>> [<ffffffff81069cba>] ? warn_slowpath_null+0x1a/0x20
> >>> [<ffffffff8107216d>] ? local_bh_enable+0x7d/0xb0
> >>> [<ffffffff81433567>] ? dev_queue_xmit+0x177/0x6c0
> >>> [<ffffffffa0293a34>] ? vlan_dev_hwaccel_hard_start_xmit+0x84/0xb0 [8021q]
> >>> [<ffffffff8142f46c>] ? dev_hard_start_xmit+0x2bc/0x3f0
> >>> [<ffffffff8143392e>] ? dev_queue_xmit+0x53e/0x6c0
> >>> [<ffffffff8142348e>] ? __skb_clone+0x2e/0x120
> >>> [<ffffffffa02ea83d>] ? fcoe_clean_pending_queue+0xcd/0x100 [libfcoe]
> >>> [<ffffffff810559ca>] ? find_busiest_group+0x9aa/0xb20
> >>> [<ffffffffa02f7624>] ? fcoe_port_send+0x24/0x50 [fcoe]
> >>> [<ffffffffa02f9568>] ? fcoe_xmit+0x228/0x3e0 [fcoe]
> >>> [<ffffffffa02c1896>] ? fc_seq_send+0xb6/0x160 [libfc]
> >>> [<ffffffffa02c1a76>] ? fc_exch_abort_locked+0x136/0x1c0 [libfc]
> >>> [<ffffffffa02c1ca7>] ? fc_exch_mgr_reset+0x147/0x2a0 [libfc]
> >>> [<ffffffff814f2c7e>] ? _spin_unlock_irq+0xe/0x20
> >>> [<ffffffffa02c91d3>] ? fc_rport_work+0x123/0x5f0 [libfc]
> >>> [<ffffffffa02c90b0>] ? fc_rport_work+0x0/0x5f0 [libfc]
> >>> [<ffffffff8108b559>] ? worker_thread+0x179/0x2a0
> >>> [<ffffffff81090ea0>] ? autoremove_wake_function+0x0/0x40
> >>> [<ffffffff8108b3e0>] ? worker_thread+0x0/0x2a0
> >>> [<ffffffff81090b56>] ? kthread+0x96/0xa0
> >>> [<ffffffff8100c10a>] ? child_rip+0xa/0x20
> >>> [<ffffffff81090ac0>] ? kthread+0x0/0xa0
> >>> [<ffffffff8100c100>] ? child_rip+0x0/0x20
> >>>
> >>>fc_exch_abort_locked attempts to send an abort frame, but it does so with a
> >>>spinlock held and irqs/bh's disabled. Because of this dev_queue_xmit issues a
> >>>warning. This patch allows the fc_exch_abort_locked to drop and re-acquire the
> >>>lock so that the warning is suppressed.
> >>>
> >>>Note this isn't a great fix, given that we need to free and re-acquire the
> >>>ex_lock during the fc_exch_abort_locked operation, but any other solution is
> >>>going to be a fairly major undertaking I think. This should work until a proper
> >>>fix can be found.
> >>>
> >>>Signed-off-by: Neil Horman<nhorman@tuxdriver.com>
> >>>Signed-off-by: Robert Love<robert.w.love@intel.com>
> >>
> >>Robert, This patch is not required based on what we discussed in
> >>this thread..
> >>http://lists.open-fcoe.org/pipermail/devel/2012-February/011946.html
> >>
> >>Neil provided "fcoe: Ensure fcoe_recv_frame is always called in
> >>process context" instead of the two patches he submitted initially.
> >>http://lists.open-fcoe.org/pipermail/devel/2012-February/011962.html
> >>
> >>Neil, correct me if I'm wrong.
> >>
> >Bhanu, This is not quite right (although that was a very confusing thread). The
> >ema_list fix that I was proposing is no longer necessecary, because after that
> >conversation we realized the whole issue cold be avoided by making sure that we
> >never allowed fcoe_recv_frame to be called in process context.
> >
> >This patch is still necessecary.
>
>
> Neil, The reason you saw this (WARNING) stack trace (in softirq.c)
> was because of using spin_lock_irqsave() instead of spin_lock_bh()
> in fc_exch_reset() as part of "[PATCH 1/2] fcoe: provide exclusion
> to concurrent ema_list access", which was not eventually submitted.
>
> Because of using spin_lock_irqsave(), the following WARN_ON is hit
> in _local_bh_enable_ip().
>
> static inline void _local_bh_enable_ip(unsigned long ip)
> {
> WARN_ON_ONCE(in_irq() || irqs_disabled());
>
> I don't think you will see this WARNING with the existing code.
>
>
> Its needed beacuse fc_exch_abort_locked is (by
> >definition) called with the ex_lock held. fc_exch_abort_locked however calls
> >fc_seq_send, which can lock the same ex_lock, causing deadlock.
>
>
> This is NOT an issue. Although fc_seq_send takes ex_lock,
> fc_exch_abort_locked() code path does not come here, as we return
> from this function if fh_type is BLS.
>
> if (fh_type == FC_TYPE_BLS)
> return error;
>
> /*
> * Update the exchange and sequence flags,
> * assuming all frames for the sequence have been sent.
> * We can only be called to send once for each sequence.
> */
> spin_lock_bh(&ep->ex_lock);
> ...
>
You're right, I apologize. The warning was erroneus, but I was thinking this
was actually a fix for a deadlock, but I'd forgotten about the fh_type check
that prevented the double lock aquisition.
Rob, this can in fact just be dropped. Thanks!
Neil
>
> Thanks,
> Bhanu
> >
> >So yes, we need a fix for this. That said, I'd just as soon you drop this
> >patch, as I was looking at this code and realized that I can make a much more
> >efficient fix for this problem. So let me know how you want to handle this: If
> >you want to drop this patch, I'll have an improved fix for you early this week.
> >If you want to keep this patch in place, thats fine with me too. My new fix
> >will just revert these bits. Let me know how you want to play it.
> >
> >Thank you for bringing this up though, I'd been meaning to review this and see
> >if I could fix it a bit better.
> >
> >Best
> >Neil
> >
> >
> >
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 02/10] fcoe: Ensure fcoe_recv_frame is always called in process context
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
2012-03-09 22:49 ` [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled Robert Love
@ 2012-03-09 22:49 ` Robert Love
2012-03-09 22:49 ` [PATCH 03/10] libfcoe: Do not sends FDISCs before FLOGI during CVL Robert Love
` (7 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:49 UTC (permalink / raw)
To: linux-scsi; +Cc: Neil Horman, Vasu Dev
From: Neil Horman <nhorman@tuxdriver.com>
commit 859b7b649ab58ee5cbfb761491317d5b315c1b0f introduced the ability to call
fcoe_recv_frame in softirq context. While this is beneficial to performance,
its not safe to do, as it breaks the serialization of access to the lport
structure (i.e. when an fcoe interface is being torn down, theres no way to
serialize the teardown effort with the completion of receieve operations
occuring in softirq context. As a result, lport (and other) data structures can
be read and modified in parallel leading to corruption. Most notable is the
vport list, which is protected by a mutex, that will cause a panic if a softirq
receive while said mutex is locked. Additionaly, the ema_list, discussed here:
http://lists.open-fcoe.org/pipermail/devel/2012-February/011947.html
Can be corrupted if a list traversal occurs in softirq context at the same time
as a list delete in process context. And generally the lport state variables
will not be stable, and may lead to unpredictable results.
The most direct fix is to remove the bits from the above commit that allowed
fcoe_recv_frame to be called in softirq context. We just force all frames to be
handled by the per-cpu rx threads. This will allow the fcoe_if_destroy's use of
fcoe_percpu_clean to function properly, ensuring that no frames are being
received while the lport is being torn down.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reviewed-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/fcoe/fcoe.c | 27 ++++++++++-----------------
1 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 2789581..22ae295 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1463,24 +1463,17 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
* so we're free to queue skbs into it's queue.
*/
- /* If this is a SCSI-FCP frame, and this is already executing on the
- * correct CPU, and the queue for this CPU is empty, then go ahead
- * and process the frame directly in the softirq context.
- * This lets us process completions without context switching from the
- * NET_RX softirq, to our receive processing thread, and then back to
- * BLOCK softirq context.
+ /*
+ * Note: We used to have a set of conditions under which we would
+ * call fcoe_recv_frame directly, rather than queuing to the rx list
+ * as it could save a few cycles, but doing so is prohibited, as
+ * fcoe_recv_frame has several paths that may sleep, which is forbidden
+ * in softirq context.
*/
- if (fh->fh_type == FC_TYPE_FCP &&
- cpu == smp_processor_id() &&
- skb_queue_empty(&fps->fcoe_rx_list)) {
- spin_unlock_bh(&fps->fcoe_rx_list.lock);
- fcoe_recv_frame(skb);
- } else {
- __skb_queue_tail(&fps->fcoe_rx_list, skb);
- if (fps->fcoe_rx_list.qlen == 1)
- wake_up_process(fps->thread);
- spin_unlock_bh(&fps->fcoe_rx_list.lock);
- }
+ __skb_queue_tail(&fps->fcoe_rx_list, skb);
+ if (fps->fcoe_rx_list.qlen == 1)
+ wake_up_process(fps->thread);
+ spin_unlock_bh(&fps->fcoe_rx_list.lock);
return 0;
err:
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 03/10] libfcoe: Do not sends FDISCs before FLOGI during CVL
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
2012-03-09 22:49 ` [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled Robert Love
2012-03-09 22:49 ` [PATCH 02/10] fcoe: Ensure fcoe_recv_frame is always called in process context Robert Love
@ 2012-03-09 22:49 ` Robert Love
2012-03-09 22:49 ` [PATCH 04/10] libfc: update fc_host mfs along with updating lport->mfs Robert Love
` (6 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:49 UTC (permalink / raw)
To: linux-scsi; +Cc: Bhanu Prakash Gollapudi
From: Bhanu Prakash Gollapudi <bprakash@broadcom.com>
When handling CVL with no Vx port descriptors, lports for NPIV ports are reset
before issuing the ctlr_reset. This causes FDISCs to be issued before
successful FLOGI. Fix it by resetting the controller before resetting the
lports.
Signed-off-by: Bhanu Prakash Gollapudi <bprakash@broadcom.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/fcoe/fcoe_ctlr.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index e7522dc..a43dad2 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -1273,11 +1273,6 @@ static void fcoe_ctlr_recv_clr_vlink(struct fcoe_ctlr *fip,
* No Vx_Port description. Clear all NPIV ports,
* followed by physical port
*/
- mutex_lock(&lport->lp_mutex);
- list_for_each_entry(vn_port, &lport->vports, list)
- fc_lport_reset(vn_port);
- mutex_unlock(&lport->lp_mutex);
-
mutex_lock(&fip->ctlr_mutex);
per_cpu_ptr(lport->dev_stats,
get_cpu())->VLinkFailureCount++;
@@ -1285,6 +1280,11 @@ static void fcoe_ctlr_recv_clr_vlink(struct fcoe_ctlr *fip,
fcoe_ctlr_reset(fip);
mutex_unlock(&fip->ctlr_mutex);
+ mutex_lock(&lport->lp_mutex);
+ list_for_each_entry(vn_port, &lport->vports, list)
+ fc_lport_reset(vn_port);
+ mutex_unlock(&lport->lp_mutex);
+
fc_lport_reset(fip->lp);
fcoe_ctlr_solicit(fip, NULL);
} else {
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 04/10] libfc: update fc_host mfs along with updating lport->mfs
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
` (2 preceding siblings ...)
2012-03-09 22:49 ` [PATCH 03/10] libfcoe: Do not sends FDISCs before FLOGI during CVL Robert Love
@ 2012-03-09 22:49 ` Robert Love
2012-03-09 22:50 ` [PATCH 05/10] libfcoe: Support extra MAC descriptor to be used as FCoE MAC Robert Love
` (5 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:49 UTC (permalink / raw)
To: linux-scsi; +Cc: Ross Brattain, Vasu Dev
From: Vasu Dev <vasu.dev@intel.com>
Currently fc_host mfs is not getting updated in
case its changed during FLOGI and that leaves fc_host
to show its initial old value in sysfs, so instead
have fc_host mfs updated along with updating lport mfs
during FLOGI.
Also in case of bad mfs during flogi, error out
instead of continuing with flogi.
[ Changes made by Robert Love: condition to '>=' and
added printing of lport->mfs in DBG statement. FLOGI
resp processing failed without being able to compare
FCoE MFS 2112 against an incoming MFS of 2112 ]
Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/libfc/fc_lport.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index 9a0b2a9..4f7ef76 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -1743,8 +1743,16 @@ void fc_lport_flogi_resp(struct fc_seq *sp, struct fc_frame *fp,
mfs = ntohs(flp->fl_csp.sp_bb_data) &
FC_SP_BB_DATA_MASK;
if (mfs >= FC_SP_MIN_MAX_PAYLOAD &&
- mfs < lport->mfs)
+ mfs <= lport->mfs) {
lport->mfs = mfs;
+ fc_host_maxframe_size(lport->host) = mfs;
+ } else {
+ FC_LPORT_DBG(lport, "FLOGI bad mfs:%hu response, "
+ "lport->mfs:%hu\n", mfs, lport->mfs);
+ fc_lport_error(lport, fp);
+ goto err;
+ }
+
csp_flags = ntohs(flp->fl_csp.sp_features);
r_a_tov = ntohl(flp->fl_csp.sp_r_a_tov);
e_d_tov = ntohl(flp->fl_csp.sp_e_d_tov);
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 05/10] libfcoe: Support extra MAC descriptor to be used as FCoE MAC
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
` (3 preceding siblings ...)
2012-03-09 22:49 ` [PATCH 04/10] libfc: update fc_host mfs along with updating lport->mfs Robert Love
@ 2012-03-09 22:50 ` Robert Love
2012-03-09 22:50 ` [PATCH 06/10] foce: remove bh disable from fcoe sw transport rcv function Robert Love
` (4 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:50 UTC (permalink / raw)
To: linux-scsi; +Cc: Bhanu Prakash Gollapudi
From: Bhanu Prakash Gollapudi <bprakash@broadcom.com>
Some switch implementations (eg., HP virtual connect FlexFabric) send two MAC
descriptors in FIP FLOGI response, with first MAC descriptor (granted_mac) used
as FPMA, and the second one (fcoe_mac) used as destination address for
sending/receiving FCoE packets. fip_mac continues to be used for FIP traffic.
This patch introduces fcoe_mac in fcoe_fcf structure. For regular switches,
both fcoe_mac and fip_mac will be the same. For the switches that send
additional MAC descriptor, fcoe_mac is updated.
Signed-off-by: Bhanu Prakash Gollapudi <bprakash@broadcom.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/fcoe/fcoe_ctlr.c | 28 +++++++++++++++++++++++-----
include/scsi/libfcoe.h | 4 +++-
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index a43dad2..249a106 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -242,7 +242,7 @@ static void fcoe_ctlr_announce(struct fcoe_ctlr *fip)
printk(KERN_INFO "libfcoe: host%d: FIP selected "
"Fibre-Channel Forwarder MAC %pM\n",
fip->lp->host->host_no, sel->fcf_mac);
- memcpy(fip->dest_addr, sel->fcf_mac, ETH_ALEN);
+ memcpy(fip->dest_addr, sel->fcoe_mac, ETH_ALEN);
fip->map_dest = 0;
}
unlock:
@@ -824,6 +824,7 @@ static int fcoe_ctlr_parse_adv(struct fcoe_ctlr *fip,
memcpy(fcf->fcf_mac,
((struct fip_mac_desc *)desc)->fd_mac,
ETH_ALEN);
+ memcpy(fcf->fcoe_mac, fcf->fcf_mac, ETH_ALEN);
if (!is_valid_ether_addr(fcf->fcf_mac)) {
LIBFCOE_FIP_DBG(fip,
"Invalid MAC addr %pM in FIP adv\n",
@@ -1013,6 +1014,7 @@ static void fcoe_ctlr_recv_els(struct fcoe_ctlr *fip, struct sk_buff *skb)
struct fip_desc *desc;
struct fip_encaps *els;
struct fcoe_dev_stats *stats;
+ struct fcoe_fcf *sel;
enum fip_desc_type els_dtype = 0;
u8 els_op;
u8 sub;
@@ -1040,7 +1042,8 @@ static void fcoe_ctlr_recv_els(struct fcoe_ctlr *fip, struct sk_buff *skb)
goto drop;
/* Drop ELS if there are duplicate critical descriptors */
if (desc->fip_dtype < 32) {
- if (desc_mask & 1U << desc->fip_dtype) {
+ if ((desc->fip_dtype != FIP_DT_MAC) &&
+ (desc_mask & 1U << desc->fip_dtype)) {
LIBFCOE_FIP_DBG(fip, "Duplicate Critical "
"Descriptors in FIP ELS\n");
goto drop;
@@ -1049,17 +1052,32 @@ static void fcoe_ctlr_recv_els(struct fcoe_ctlr *fip, struct sk_buff *skb)
}
switch (desc->fip_dtype) {
case FIP_DT_MAC:
+ sel = fip->sel_fcf;
if (desc_cnt == 1) {
LIBFCOE_FIP_DBG(fip, "FIP descriptors "
"received out of order\n");
goto drop;
}
+ /*
+ * Some switch implementations send two MAC descriptors,
+ * with first MAC(granted_mac) being the FPMA, and the
+ * second one(fcoe_mac) is used as destination address
+ * for sending/receiving FCoE packets. FIP traffic is
+ * sent using fip_mac. For regular switches, both
+ * fip_mac and fcoe_mac would be the same.
+ */
+ if (desc_cnt == 2)
+ memcpy(granted_mac,
+ ((struct fip_mac_desc *)desc)->fd_mac,
+ ETH_ALEN);
if (dlen != sizeof(struct fip_mac_desc))
goto len_err;
- memcpy(granted_mac,
- ((struct fip_mac_desc *)desc)->fd_mac,
- ETH_ALEN);
+
+ if ((desc_cnt == 3) && (sel))
+ memcpy(sel->fcoe_mac,
+ ((struct fip_mac_desc *)desc)->fd_mac,
+ ETH_ALEN);
break;
case FIP_DT_FLOGI:
case FIP_DT_FDISC:
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 5a35a2a..cfdb55f 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -165,7 +165,8 @@ struct fcoe_ctlr {
* @switch_name: WWN of switch from advertisement
* @fabric_name: WWN of fabric from advertisement
* @fc_map: FC_MAP value from advertisement
- * @fcf_mac: Ethernet address of the FCF
+ * @fcf_mac: Ethernet address of the FCF for FIP traffic
+ * @fcoe_mac: Ethernet address of the FCF for FCoE traffic
* @vfid: virtual fabric ID
* @pri: selection priority, smaller values are better
* @flogi_sent: current FLOGI sent to this FCF
@@ -188,6 +189,7 @@ struct fcoe_fcf {
u32 fc_map;
u16 vfid;
u8 fcf_mac[ETH_ALEN];
+ u8 fcoe_mac[ETH_ALEN];
u8 pri;
u8 flogi_sent;
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 06/10] foce: remove bh disable from fcoe sw transport rcv function
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
` (4 preceding siblings ...)
2012-03-09 22:50 ` [PATCH 05/10] libfcoe: Support extra MAC descriptor to be used as FCoE MAC Robert Love
@ 2012-03-09 22:50 ` Robert Love
2012-03-09 22:50 ` [PATCH 07/10] bnx2fc: Remove bh disable in softirq context Robert Love
` (3 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:50 UTC (permalink / raw)
To: linux-scsi; +Cc: Neil Horman, Vasu Dev
From: Neil Horman <nhorman@tuxdriver.com>
The fcoe sw recive packet function (fcoe_rcv) only ever executes in softirq
context. Given that, and the fact that no use of the fcoe_rx_list is made in
irq context, its not necessecary to disable bottom halves while actually
receiving the frame. Convert spin_*_bh calls in that function to their
lock-only equivalents
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/fcoe/fcoe.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 22ae295..961bf36 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1436,7 +1436,7 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
goto err;
fps = &per_cpu(fcoe_percpu, cpu);
- spin_lock_bh(&fps->fcoe_rx_list.lock);
+ spin_lock(&fps->fcoe_rx_list.lock);
if (unlikely(!fps->thread)) {
/*
* The targeted CPU is not ready, let's target
@@ -1447,12 +1447,12 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
"ready for incoming skb- using first online "
"CPU.\n");
- spin_unlock_bh(&fps->fcoe_rx_list.lock);
+ spin_unlock(&fps->fcoe_rx_list.lock);
cpu = cpumask_first(cpu_online_mask);
fps = &per_cpu(fcoe_percpu, cpu);
- spin_lock_bh(&fps->fcoe_rx_list.lock);
+ spin_lock(&fps->fcoe_rx_list.lock);
if (!fps->thread) {
- spin_unlock_bh(&fps->fcoe_rx_list.lock);
+ spin_unlock(&fps->fcoe_rx_list.lock);
goto err;
}
}
@@ -1473,7 +1473,7 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
__skb_queue_tail(&fps->fcoe_rx_list, skb);
if (fps->fcoe_rx_list.qlen == 1)
wake_up_process(fps->thread);
- spin_unlock_bh(&fps->fcoe_rx_list.lock);
+ spin_unlock(&fps->fcoe_rx_list.lock);
return 0;
err:
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 07/10] bnx2fc: Remove bh disable in softirq context
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
` (5 preceding siblings ...)
2012-03-09 22:50 ` [PATCH 06/10] foce: remove bh disable from fcoe sw transport rcv function Robert Love
@ 2012-03-09 22:50 ` Robert Love
2012-03-09 22:50 ` [PATCH 08/10] fcoe: remove frame dropping code from fcoe_percpu_clean Robert Love
` (2 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:50 UTC (permalink / raw)
To: linux-scsi; +Cc: Neil Horman, Bhanu Prakash Gollapudi
From: Neil Horman <nhorman@tuxdriver.com>
As with the fcoe sw transport, the bnx2fc packet handler function runs only in
softirq context. Theres no need to disable bottom halves here
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: Bhanu Prakash Gollapudi <bprakash@broadcom.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 796fd30..1c34a44 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -440,13 +440,13 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
fr->fr_dev = lport;
bg = &bnx2fc_global;
- spin_lock_bh(&bg->fcoe_rx_list.lock);
+ spin_lock(&bg->fcoe_rx_list.lock);
__skb_queue_tail(&bg->fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
wake_up_process(bg->thread);
- spin_unlock_bh(&bg->fcoe_rx_list.lock);
+ spin_unlock(&bg->fcoe_rx_list.lock);
return 0;
err:
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 08/10] fcoe: remove frame dropping code from fcoe_percpu_clean
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
` (6 preceding siblings ...)
2012-03-09 22:50 ` [PATCH 07/10] bnx2fc: Remove bh disable in softirq context Robert Love
@ 2012-03-09 22:50 ` Robert Love
2012-03-09 22:50 ` [PATCH 09/10] fcoe: reduce contention for fcoe_rx_list lock [v2] Robert Love
2012-03-09 22:50 ` [PATCH 10/10] libfc: fcoe_transport_create fails in single-CPU environment Robert Love
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:50 UTC (permalink / raw)
To: linux-scsi; +Cc: James E.J. Bottomley, Vasu Dev, Neil Horman, Vasu Dev
From: Neil Horman <nhorman@tuxdriver.com>
commit e7a51997dad4e17395be1209970e18d2e9305b24 added a skb flush to the
fcoe_rx_list, which ensures that we push any pending frames on the list through
the per-cpu receive thread. Because of this, its redundant to lock and scan the
list first, dropping any arriving frames.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Robert Love <robert.w.love@intel.com>
CC: Vasu Dev <vasu.dev@linux.intel.com>
CC: "James E.J. Bottomley" <JBottomley@parallels.com>
Acked-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/fcoe/fcoe.c | 22 +++-------------------
1 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 961bf36..d86ca37 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -2254,31 +2254,14 @@ static int fcoe_link_ok(struct fc_lport *lport)
static void fcoe_percpu_clean(struct fc_lport *lport)
{
struct fcoe_percpu_s *pp;
- struct fcoe_rcv_info *fr;
- struct sk_buff_head *list;
- struct sk_buff *skb, *next;
- struct sk_buff *head;
+ struct sk_buff *skb;
unsigned int cpu;
for_each_possible_cpu(cpu) {
pp = &per_cpu(fcoe_percpu, cpu);
- spin_lock_bh(&pp->fcoe_rx_list.lock);
- list = &pp->fcoe_rx_list;
- head = list->next;
- for (skb = head; skb != (struct sk_buff *)list;
- skb = next) {
- next = skb->next;
- fr = fcoe_dev_from_skb(skb);
- if (fr->fr_dev == lport) {
- __skb_unlink(skb, list);
- kfree_skb(skb);
- }
- }
- if (!pp->thread || !cpu_online(cpu)) {
- spin_unlock_bh(&pp->fcoe_rx_list.lock);
+ if (!pp->thread || !cpu_online(cpu))
continue;
- }
skb = dev_alloc_skb(0);
if (!skb) {
@@ -2287,6 +2270,7 @@ static void fcoe_percpu_clean(struct fc_lport *lport)
}
skb->destructor = fcoe_percpu_flush_done;
+ spin_lock_bh(&pp->fcoe_rx_list.lock);
__skb_queue_tail(&pp->fcoe_rx_list, skb);
if (pp->fcoe_rx_list.qlen == 1)
wake_up_process(pp->thread);
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 09/10] fcoe: reduce contention for fcoe_rx_list lock [v2]
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
` (7 preceding siblings ...)
2012-03-09 22:50 ` [PATCH 08/10] fcoe: remove frame dropping code from fcoe_percpu_clean Robert Love
@ 2012-03-09 22:50 ` Robert Love
2012-03-09 22:50 ` [PATCH 10/10] libfc: fcoe_transport_create fails in single-CPU environment Robert Love
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:50 UTC (permalink / raw)
To: linux-scsi; +Cc: Neil Horman, Vasu Dev
From: Neil Horman <nhorman@tuxdriver.com>
There is potentially lots of contention for the rx_list_lock. On a cpu that is
receiving lots of fcoe traffic, the softirq context has to add and release the
lock for every frame it receives, as does the receiving per-cpu thread. We can
reduce this contention somewhat by altering the per-cpu threads loop such that
when traffic is detected on the fcoe_rx_list, we splice it to a temporary list.
In this way, we can process multiple skbs while only having to acquire and
release the fcoe_rx_list lock once.
[ Braces around single statement while loop removed by Robert Love
to satisfy checkpath.pl. ]
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/fcoe/fcoe.c | 22 ++++++++++++++--------
1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index d86ca37..58c88b0 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1471,7 +1471,7 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
* in softirq context.
*/
__skb_queue_tail(&fps->fcoe_rx_list, skb);
- if (fps->fcoe_rx_list.qlen == 1)
+ if (fps->thread->state == TASK_INTERRUPTIBLE)
wake_up_process(fps->thread);
spin_unlock(&fps->fcoe_rx_list.lock);
@@ -1790,23 +1790,29 @@ static int fcoe_percpu_receive_thread(void *arg)
{
struct fcoe_percpu_s *p = arg;
struct sk_buff *skb;
+ struct sk_buff_head tmp;
+
+ skb_queue_head_init(&tmp);
set_user_nice(current, -20);
while (!kthread_should_stop()) {
spin_lock_bh(&p->fcoe_rx_list.lock);
- while ((skb = __skb_dequeue(&p->fcoe_rx_list)) == NULL) {
+ skb_queue_splice_init(&p->fcoe_rx_list, &tmp);
+ spin_unlock_bh(&p->fcoe_rx_list.lock);
+
+ while ((skb = __skb_dequeue(&tmp)) != NULL)
+ fcoe_recv_frame(skb);
+
+ spin_lock_bh(&p->fcoe_rx_list.lock);
+ if (!skb_queue_len(&p->fcoe_rx_list)) {
set_current_state(TASK_INTERRUPTIBLE);
spin_unlock_bh(&p->fcoe_rx_list.lock);
schedule();
set_current_state(TASK_RUNNING);
- if (kthread_should_stop())
- return 0;
- spin_lock_bh(&p->fcoe_rx_list.lock);
- }
- spin_unlock_bh(&p->fcoe_rx_list.lock);
- fcoe_recv_frame(skb);
+ } else
+ spin_unlock_bh(&p->fcoe_rx_list.lock);
}
return 0;
}
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 10/10] libfc: fcoe_transport_create fails in single-CPU environment
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
` (8 preceding siblings ...)
2012-03-09 22:50 ` [PATCH 09/10] fcoe: reduce contention for fcoe_rx_list lock [v2] Robert Love
@ 2012-03-09 22:50 ` Robert Love
9 siblings, 0 replies; 17+ messages in thread
From: Robert Love @ 2012-03-09 22:50 UTC (permalink / raw)
To: linux-scsi; +Cc: Steven Clark
From: Steven Clark <sclark@crossbeam.com>
Starting fcoe fails at fcoe_transport_create when attempting to allocate a pool of 4K exchanges on a
64-bit single-CPU environment because the call to __alloc_percpu() is greater than the max of 32K.
This patch reduces the number of exchanges to fit within the maximum allowed space.
[ Whitespace problems fixed by Robert Love to satisfy chechpatch.pl ]
Signed-off-by: Steven Clark <sclark@crossbeam.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
drivers/scsi/libfc/fc_exch.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 588060f..31cf794 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -2284,7 +2284,18 @@ struct fc_exch_mgr *fc_exch_mgr_alloc(struct fc_lport *lport,
mp->class = class;
/* adjust em exch xid range for offload */
mp->min_xid = min_xid;
- mp->max_xid = max_xid;
+
+ /* reduce range so per cpu pool fits into PCPU_MIN_UNIT_SIZE pool */
+ pool_exch_range = (PCPU_MIN_UNIT_SIZE - sizeof(*pool)) /
+ sizeof(struct fc_exch *);
+ if ((max_xid - min_xid + 1) / (fc_cpu_mask + 1) > pool_exch_range) {
+ mp->max_xid = pool_exch_range * (fc_cpu_mask + 1) +
+ min_xid - 1;
+ } else {
+ mp->max_xid = max_xid;
+ pool_exch_range = (mp->max_xid - mp->min_xid + 1) /
+ (fc_cpu_mask + 1);
+ }
mp->ep_pool = mempool_create_slab_pool(2, fc_em_cachep);
if (!mp->ep_pool)
@@ -2295,7 +2306,6 @@ struct fc_exch_mgr *fc_exch_mgr_alloc(struct fc_lport *lport,
* divided across all cpus. The exch pointers array memory is
* allocated for exch range per pool.
*/
- pool_exch_range = (mp->max_xid - mp->min_xid + 1) / (fc_cpu_mask + 1);
mp->pool_max_index = pool_exch_range - 1;
/*
^ permalink raw reply related [flat|nested] 17+ messages in thread